Bug 63159 - daemon: Add wtmp file monitor
Summary: daemon: Add wtmp file monitor
Status: RESOLVED FIXED
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-05 11:43 UTC by Ondrej Holy
Modified: 2013-06-06 00:26 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
add wtmp file monitor (2.71 KB, text/plain)
2013-04-05 11:43 UTC, Ondrej Holy
Details
rename on_passwd_monitor_changed (2.30 KB, patch)
2013-05-24 10:52 UTC, Ondrej Holy
Details | Splinter Review
check for monitor error immediately (3.37 KB, patch)
2013-05-24 10:52 UTC, Ondrej Holy
Details | Splinter Review
add wtmp file monitor (2.39 KB, patch)
2013-05-24 10:53 UTC, Ondrej Holy
Details | Splinter Review
Emit "changed" signal when setting information from wtmp. (1.98 KB, patch)
2013-05-30 06:57 UTC, Marius Vollmer
Details | Splinter Review

Description Ondrej Holy 2013-04-05 11:43:41 UTC
Created attachment 77471 [details]
add wtmp file monitor

File monitor should be added for wtmp file to have current user history.

More info: https://bugzilla.gnome.org/show_bug.cgi?id=697040

I've attached patch to do that.
Comment 1 Ray Strode [halfline] 2013-05-14 23:28:24 UTC
> +#ifdef HAVE_UTMPX_H
> +#define PATH_UTMPX _PATH_WTMPX
Should be called PATH_WTMP, since it's the wtmp file not the utmp file

> +#ifdef HAVE_UTMPX_H
> +        file = g_file_new_for_path (PATH_UTMPX);
> +        daemon->priv->utmpx_monitor = g_file_monitor_file (file,
> +                                                           G_FILE_MONITOR_NONE,
> +                                                           NULL,
> +                                                           &error);
> +        g_object_unref (file);
> +#endif
This highlights an existing problem in the code that would be good to fix in a prerequisite commit.  The various monitors aren't checked for error until way later in the function and so their errors can stomp on top of each other.

>  
>          if (daemon->priv->passwd_monitor != NULL) {
>                  g_signal_connect (daemon->priv->passwd_monitor,
> @@ -801,6 +815,17 @@ daemon_init (Daemon *daemon)
>                  g_warning ("Unable to monitor %s: %s", PATH_GDM_CUSTOM, error->message);
>                  g_error_free (error);
>          }
> +#ifdef HAVE_UTMPX_H
> +        if (daemon->priv->utmpx_monitor != NULL) {
> +                g_signal_connect (daemon->priv->utmpx_monitor,
> +                                  "changed",
> +                                  G_CALLBACK (on_passwd_monitor_changed),
This highlights another preexisting problem with the code. the function is named on_passwd_monitor_changed, but it's called for all the monitors.  It would be good if there was a preliminary commit that renamed it (maybe on_users_monitor_changed ?).
Comment 2 Ondrej Holy 2013-05-24 10:52:10 UTC
Created attachment 79756 [details] [review]
rename on_passwd_monitor_changed
Comment 3 Ondrej Holy 2013-05-24 10:52:48 UTC
Created attachment 79757 [details] [review]
check for monitor error immediately
Comment 4 Ondrej Holy 2013-05-24 10:53:10 UTC
Created attachment 79758 [details] [review]
add wtmp file monitor
Comment 5 Marius Vollmer 2013-05-30 06:57:20 UTC
Created attachment 80014 [details] [review]
Emit "changed" signal when setting information from wtmp.

I also found this patch to be necessary when reloading users on wtmp changes.
Comment 6 Matthias Clasen 2013-06-06 00:20:05 UTC
Comment on attachment 79756 [details] [review]
rename on_passwd_monitor_changed

Review of attachment 79756 [details] [review]:
-----------------------------------------------------------------

looks good


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.