[Spice-devel] [vdagent-linux v3 2/4] console-kit: implement check for locked session

[Jonathon Jongsma]

On Thu, 2016-04-28 at 15:37 +0200, Victor Toso wrote:
> We register to read the Lock, Unlock and IdleHintChanged signals from
> ConsoleKit.Session. The Lock/Unlock signals should be the right
> signals for the job but not all Desktop Managers implement its locking
> in a way that trigger those signals. That's why we double-check with
> IdleHintChanged signal that it might be triggered by other services
> like screen savers.
> 
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>  src/console-kit.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> -
>  1 file changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/src/console-kit.c b/src/console-kit.c
> index 72a5b16..b21df6f 100644
> --- a/src/console-kit.c
> +++ b/src/console-kit.c
> @@ -35,6 +35,9 @@ struct session_info {
>      char *active_session;
>      int verbose;
>      gchar *match_seat_signals;
> +    gchar *match_session_signals;
> +    gboolean session_is_locked;
> +    gboolean session_idle_hint;
>  };
>  
>  #define INTERFACE_CONSOLE_KIT "org.freedesktop.ConsoleKit"
> @@ -45,8 +48,15 @@ struct session_info {
>  
>  #define INTERFACE_CONSOLE_KIT_SEAT       INTERFACE_CONSOLE_KIT ".Seat"
>  
> +#define INTERFACE_CONSOLE_KIT_SESSION    INTERFACE_CONSOLE_KIT ".Session"
> +#define OBJ_PATH_CONSOLE_KIT_SESSION     OBJ_PATH_CONSOLE_KIT "/Session"
> +
>  #define SEAT_SIGNAL_ACTIVE_SESSION_CHANGED       "ActiveSessionChanged"
>  
> +#define SESSION_SIGNAL_LOCK                      "Lock"
> +#define SESSION_SIGNAL_UNLOCK                    "Unlock"
> +#define SESSION_SIGNAL_IDLE_HINT_CHANGED         "IdleHintChanged"
> +
>  static char *console_kit_get_first_seat(struct session_info *info);
>  static char *console_kit_check_active_session_change(struct session_info
> *info);
>  
> @@ -64,6 +74,19 @@ static void si_dbus_match_remove(struct session_info *info)
>          g_free(info->match_seat_signals);
>          info->match_seat_signals = NULL;
>      }
> +
> +    if (info->match_session_signals != NULL) {
> +        dbus_error_init(&error);
> +        dbus_bus_remove_match(info->connection,
> +                              info->match_session_signals,
> +                              &error);
> +
> +        if (info->verbose)
> +            syslog(LOG_DEBUG, "(console-kit) session match removed: %s",
> +                   info->match_session_signals);
> +        g_free(info->match_session_signals);
> +        info->match_session_signals = NULL;
> +    }
>  }
>  
>  static void si_dbus_match_rule_update(struct session_info *info)
> @@ -97,6 +120,28 @@ static void si_dbus_match_rule_update(struct session_info
> *info)
>              g_free(info->match_seat_signals);
>          }
>      }
> +
> +    /* Session signals */
> +    if (info->active_session != NULL) {
> +        info->match_session_signals =
> +            g_strdup_printf ("type='signal',interface='%s',path='%s'",
> +                             INTERFACE_CONSOLE_KIT_SESSION,
> +                             info->active_session);
> +        if (info->verbose)
> +            syslog(LOG_DEBUG, "(console-kit) session match: %s",
> +                   info->match_session_signals);
> +
> +        dbus_error_init(&error);
> +        dbus_bus_add_match(info->connection,
> +                           info->match_session_signals,
> +                           &error);
> +        if (dbus_error_is_set(&error)) {
> +            syslog(LOG_WARNING, "Unable to add dbus rule match: %s",
> +                   error.message);
> +            dbus_error_free(&error);
> +            g_free(info->match_session_signals);

Dangling pointer. Set to NULL.

> +        }
> +    }
>  }
>  
>  static void
> @@ -133,6 +178,7 @@ si_dbus_read_signals(struct session_info *info)
>                  dbus_message_iter_get_basic(&iter, &session);
>                  if (session != NULL && session[0] != '\0') {
>                      info->active_session = g_strdup(session);
> +                    si_dbus_match_rule_update(info);
>                  } else {
>                      syslog(LOG_WARNING, "(console-kit) received invalid
> session. "
>                             "No active-session at the moment");
> @@ -142,6 +188,25 @@ si_dbus_read_signals(struct session_info *info)
>                         "ActiveSessionChanged message has unexpected type:
> '%c'",
>                         type);
>              }
> +        } else if (g_strcmp0(member, SESSION_SIGNAL_LOCK) == 0) {
> +            info->session_is_locked = TRUE;
> +        } else if (g_strcmp0(member, SESSION_SIGNAL_UNLOCK) == 0) {
> +            info->session_is_locked = FALSE;
> +        } else if (g_strcmp0(member, SESSION_SIGNAL_IDLE_HINT_CHANGED) == 0)
> {
> +            DBusMessageIter iter;
> +            gint type;
> +            dbus_bool_t idle_hint;
> +
> +            dbus_message_iter_init(message, &iter);
> +            type = dbus_message_iter_get_arg_type(&iter);
> +            if (type == DBUS_TYPE_BOOLEAN) {
> +                dbus_message_iter_get_basic(&iter, &idle_hint);
> +                info->session_idle_hint = (idle_hint);
> +            } else {
> +                syslog(LOG_ERR,
> +                       "(console-kit) IdleHintChanged has unexpected type:
> '%c'",
> +                       type);
> +            }
>          } else if (info->verbose) {
>              syslog(LOG_DEBUG, "(console-kit) Signal not handled: %s",
> member);
>          }
> @@ -162,6 +227,8 @@ struct session_info *session_info_create(int verbose)
>          return NULL;
>  
>      info->verbose = verbose;
> +    info->session_is_locked = FALSE;
> +    info->session_idle_hint = FALSE;
>  
>      dbus_error_init(&error);
>      info->connection = dbus_bus_get_private(DBUS_BUS_SYSTEM, &error);
> @@ -324,6 +391,7 @@ const char *session_info_get_active_session(struct
> session_info *info)
>      }
>  
>      info->active_session = strdup(session);
> +    si_dbus_match_rule_update(info);
>  
>  exit:
>      if (reply != NULL) {
> @@ -419,7 +487,18 @@ static char
> *console_kit_check_active_session_change(struct session_info *info)
>  
>  gboolean session_info_session_is_locked(struct session_info *info)
>  {
> -    /* TODO: It could be implemented based on Lock/Unlock signals from
> Session
> -     * interface. */
> -    return FALSE;
> +    if (info == NULL)
> +        return FALSE;

This seems like an unexpected situation? Perhaps use something like
g_return_val_if_fail()? Do we really want to default to reporting 'unlocked' in
this scenario? reporting it as 'locked' seems somewhat safer

> +
> +    /* Not every system does emit Lock and Unlock signals (for instance, such
> +     * is the case for RHEL6) but most of the systems seems to emit the
> +     * IdleHintChanged. So, we give priority to the Lock signal, if it is
> Locked
> +     * we return that the session is locked, otherwise we double check with
> the
> +     * IdleHint value */
> +    si_dbus_read_signals(info);
> +    if (info->verbose) {
> +        syslog(LOG_DEBUG, "(console-kit) session is locked: %s",
> +               (info->session_is_locked || info->session_idle_hint) ? "yes" :
> "no");
> +    }
> +    return (info->session_is_locked || info->session_idle_hint);

Personal preference. I'd introduce a local boolean variable here so that you
don't need to repeat the (info->session_is_locked || info->session_idle_hint)
twice (once for the debug statement and once for the return value)


Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>