Bug 39295

Created attachment 49197 [details] [review]
the patch

Review of attachment 49197 [details] [review]:

First off, I really like the idea of this patch.  I've never really liked parsing the output of a program and this all around simplifies the code.  It sort of made sense when ConsoleKit was vying to replace utmp, but now that ConsoleKit is fading away into the distance this seems very appropriate.

::: src/daemon.c
@@ +250,3 @@
+        GHashTable *h;
+        GHashTableIter i;
+        gpointer k, c;

should probably use better variable names than u, h, i, k, c etc.

@@ +272,3 @@
 
+                c = g_hash_table_lookup (h, u->ut_user);
+                counter = GPOINTER_TO_UINT (c) + 1;

It's a little tr/icky to use the NULL return value from a "not found" look up as 0 for the counter.  Not a big deal, though.

@@ +277,1 @@
+                g_hash_table_replace (h, g_strdup(u->ut_user), c);

So here we're potentially calling g_strdup/g_free repeatedly for users who have logged in a bunch.  Again not a big deal, but i have an idea...

1) Pass NULL from the key free func to the hash table constructor
2) Use lookup_extended to test whether the key already exists in the hash or not.
3) if it doesn't exist, then g_hash_table_insert the strdup'd key and set c = 0
4) if it does exist, then g_hash_table_insert without strduping the key, and set c to the returned value
5) add a g_hash_table_foreach (h, g_free, NULL) afterward to clean up.

@@ +295,1 @@
+        endutxent ();

This should be done before g_hash_table_iter_init

::: src/libaccountsservice/act-user.c
@@ +858,3 @@
  * with it.
  *
+ * Returns: (transfer none): the object path of the user

ah this is a separate fix, should go in separately

Review of attachment 49197 [details] [review]:

First off, I really like the idea of this patch.  I've never really liked parsing the output of a program and this all around simplifies the code.  It sort of made sense when ConsoleKit was vying to replace utmp, but now that ConsoleKit is fading away into the distance this seems very appropriate.

::: src/daemon.c
@@ +250,3 @@
+        GHashTable *h;
+        GHashTableIter i;
+        gpointer k, c;

should probably use better variable names than u, h, i, k, c etc.

@@ +272,3 @@
 
+                c = g_hash_table_lookup (h, u->ut_user);
+                counter = GPOINTER_TO_UINT (c) + 1;

It's a little tr/icky to use the NULL return value from a "not found" look up as 0 for the counter.  Not a big deal, though.

@@ +277,1 @@
+                g_hash_table_replace (h, g_strdup(u->ut_user), c);

So here we're potentially calling g_strdup/g_free repeatedly for users who have logged in a bunch.  Again not a big deal, but i have an idea...

1) Pass NULL from the key free func to the hash table constructor
2) Use lookup_extended to test whether the key already exists in the hash or not.
3) if it doesn't exist, then g_hash_table_insert the strdup'd key and set c = 0
4) if it does exist, then g_hash_table_insert without strduping the key, and set c to the returned value
5) add a g_hash_table_foreach (h, g_free, NULL) afterward to clean up.

@@ +295,1 @@
+        endutxent ();

This should be done before g_hash_table_iter_init

::: src/libaccountsservice/act-user.c
@@ +858,3 @@
  * with it.
  *
+ * Returns: (transfer none): the object path of the user

ah this is a separate fix, should go in separately

ignore the duplicate review.  I ran into a Splinter bug there.

I'll go ahead and make the minor changes I suggested and push your patch.

http://cgit.freedesktop.org/accountsservice/commit/?id=281ac4126636a7a701b95c465ab9de7762e17fdf

Hmm, this doesn't look right.

When you add the entry to the hash table at first you need to set the counter to 1, not 0, to count logins (not that it really made much of a difference effectively, but i'd say that that 1 login should encoded a "1"...).

+ value = GUINT_TO_POINTER (0);
+ g_hash_table_insert (login_frequency_hash,
+         g_strdup (wtmp_entry->ut_user),
+         GUINT_TO_POINTER (0));

In that code you assign a value to "value", but don't use it. You can just drop the assignment.

http://cgit.freedesktop.org/accountsservice/commit/?id=a4e478b9fef287698a3369d4b0d779c8ab949ff3

Oh, dang. One more thing I missed. You replaced a g_hash_table_replace() by g_hash_table_insert(). That means the increased counter will never be written back into the hash table, since the key is already in there. The _replace() is important to get the value actually updated.

Sorry for not noticing that earlier!

(In reply to comment #8)
> Oh, dang. One more thing I missed. You replaced a g_hash_table_replace() by
> g_hash_table_insert(). That means the increased counter will never be written
> back into the hash table, since the key is already in there. The _replace() is
> important to get the value actually updated.
> 
> Sorry for not noticing that earlier!

Nope, g_hash_table_insert replaces as well. the only difference is what happens to the key, not the value.

i've released 0.6.13 with this change now:

http://www.freedesktop.org/software/accountsservice/