Hi,
On Fri, Apr 29, 2016 at 10:08:53AM -0500, Jonathon Jongsma wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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.
Ok!
>
> > + }
> > + }
> > }
> >
> > 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()?
Agreed!
> Do we really want to default to reporting 'unlocked' in this scenario?
> reporting it as 'locked' seems somewhat safer
The reason is that I was a bit afraid of changing the behavior in case of
unforeseen problems. As this should only happen in a locked-screen and this
scenario is likely a corner case in the usage of the file-xfer (and possible
features that we want to disable in the future), I think defaulting to
'unlocked' is somewhat safer but I could be wrong.
>
> > +
> > + /* 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)
I'll do that!
toso
>
>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel