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

[Victor Toso]

Hi,

On Fri, Apr 29, 2016 at 02:52:03PM -0500, Jonathon Jongsma wrote:
> On Thu, 2016-04-28 at 15:37 +0200, Victor Toso wrote:
> > Implementing the function that was introduced in previous commit:
> > session_info_is_user()
> > 
> > In console-kit we can gather the session type with GetSessionType api
> > from Session interface. This function is not well documented but seems
> > reliable enough as GDM was using it [0]
> > 
> > [0] session_is_login_window on gui/simple-greeter/gdm-user-manager.c
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1323640
> > ---
> >  src/console-kit.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > -
> >  1 file changed, 66 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/console-kit.c b/src/console-kit.c
> > index 036234e..182e886 100644
> > --- a/src/console-kit.c
> > +++ b/src/console-kit.c
> > @@ -503,8 +503,72 @@ gboolean session_info_session_is_locked(struct
> > session_info *info)
> >      return (info->session_is_locked || info->session_idle_hint);
> >  }
> >  
> > +/* This function should only be called after session_info_get_active_session
> > + * in order to verify if active session belongs to user (non greeter) */
> >  gboolean session_info_is_user(struct session_info *info)
> >  {
> > -    /* TODO */
> > -    return TRUE;
> > +    DBusError error;
> > +    DBusMessage *message;
> > +    DBusMessage *reply;
>
> would prefer to initialize message and reply to NULL just in case at some point
> the code changes and there is a 'goto exit' call before one of these variables
> gets assigned.

Sure, I'll change it.

>
> > +    gchar *session_type = NULL;
> > +    gboolean ret = TRUE;
> > +
> > +    if (info == NULL || info->connection == NULL || info->active_session ==
> > NULL)
> > +        return TRUE;
>
> Here again, we're defaulting to permissive. As in the previous reviews, should
> we default to restrictive? Maybe not, but I just wanted to mention it.

Sure. Let me know what you think from my replies in previous patches.

> > +
> > +    message = dbus_message_new_method_call(INTERFACE_CONSOLE_KIT,
> > +                                           info->active_session,
> > +                                           INTERFACE_CONSOLE_KIT_SESSION,
> > +                                           "GetSessionType");
> > +    if (message == NULL) {
> > +        syslog(LOG_ERR,
> > +               "(console-kit) Unable to create dbus message for
> > GetSessionType");
> > +        return TRUE;
> > +    }
> > +
> > +    dbus_error_init(&error);
> > +    reply = dbus_connection_send_with_reply_and_block(info->connection,
> > +                                                      message,
> > +                                                      -1,
> > +                                                      &error);
> > +    if (reply == NULL || dbus_error_is_set(&error)) {
> > +        if (dbus_error_is_set(&error)) {
> > +            syslog(LOG_ERR, "GetSessionType failed: %s", error.message);
> > +            dbus_error_free(&error);
> > +        } else
> > +            syslog(LOG_ERR, "GetSessionType failed");
> > +        goto exit;
> > +    }
> > +
> > +    dbus_error_init(&error);
> > +    if (!dbus_message_get_args(reply,
> > +                               &error,
> > +                               DBUS_TYPE_STRING, &session_type,
> > +                               DBUS_TYPE_INVALID)) {
> > +        if (dbus_error_is_set(&error)) {
> > +            syslog(LOG_ERR,
> > +                   "(console-kit) fail to get session-type from reply: %s",
> > +                   error.message);
> > +            dbus_error_free(&error);
> > +        } else {
> > +            syslog(LOG_ERR, "(console-kit) fail to get session-type from
> > reply");
> > +        }
> > +        session_type = NULL;
> > +        goto exit;
> > +    }
> > +
> > +    /* Empty session_type means user */
> > +    if (info->verbose)
> > +        syslog(LOG_DEBUG, "(console-kit) session-type is '%s'",
> > session_type);
> > +
> > +    ret = (g_strcmp0 (session_type, "LoginWindow") != 0);
> > +
> > +exit:
> > +    if (reply != NULL) {
> > +        dbus_message_unref(reply);
> > +    }
> > +    if (message != NULL) {
> > +        dbus_message_unref(message);
> > +    }
> > +    return ret;
> >  }
>
> Otherwise it looks ok.
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>

Many thanks for the reviews. I'll send the v4 after we decide if we should
change the session_info_session_is_locked to return TRUE instead of FALSE as
default and session_info_is_user to return FALSE instead of TRUE as default.

Cheers,
  toso

> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel