Bug 39295

Summary: [PATCH] use utmpx instead of CK history for user login frequencies
Product: accountsservice Reporter: Lennart Poettering <lennart>
Component: generalAssignee: Matthias Clasen <mclasen>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: the patch

Description Lennart Poettering 2011-07-16 18:42:21 UTC
Use the POSIX utmpx database instead of ConsoleKit as source for log-in data. This is more portable and much simpler. Also, it allows us to prepare accountsservice for the post-CK times.
Comment 1 Lennart Poettering 2011-07-16 18:44:52 UTC
Created attachment 49197 [details] [review]
the patch
Comment 2 Ray Strode [halfline] 2011-07-19 08:33:58 UTC
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
Comment 3 Ray Strode [halfline] 2011-07-19 08:33:58 UTC
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
Comment 4 Ray Strode [halfline] 2011-07-19 08:35:21 UTC
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.
Comment 6 Lennart Poettering 2011-07-19 11:54:10 UTC
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.
Comment 8 Lennart Poettering 2011-07-19 12:46:51 UTC
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!
Comment 9 Ray Strode [halfline] 2011-07-19 12:49:24 UTC
(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.
Comment 10 Ray Strode [halfline] 2011-07-19 12:55:49 UTC
i've released 0.6.13 with this change now:

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

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.