Bug 63159

Summary: daemon: Add wtmp file monitor
Product: accountsservice Reporter: Ondrej Holy <oholy>
Component: generalAssignee: Matthias Clasen <mclasen>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: rstrode
Version: unspecified   
Hardware: Other   
OS: All   
i915 platform: i915 features:
Attachments: add wtmp file monitor
rename on_passwd_monitor_changed
check for monitor error immediately
add wtmp file monitor
Emit "changed" signal when setting information from wtmp.

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
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.