[Spice-devel] [vdagent-linux v2 5/6] console-kit: include signal handler function

[Jonathon Jongsma]

On Sat, 2016-04-23 at 12:27 +0200, Victor Toso wrote:
> At the moment we are only reading the ActiveSessionChanged signal from
> ConsoleKit.Seat but in a later patch we will be reading a few more signals so
> it
> make sense to move this handler to its own function.
> ---
>  src/console-kit.c | 111 ++++++++++++++++++++++++++++-------------------------
> -
>  1 file changed, 57 insertions(+), 54 deletions(-)
> 
> diff --git a/src/console-kit.c b/src/console-kit.c
> index 7096609..41a1cfe 100644
> --- a/src/console-kit.c
> +++ b/src/console-kit.c
> @@ -26,6 +26,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <syslog.h>
> +#include <glib.h>
>  
>  struct session_info {
>      DBusConnection *connection;
> @@ -43,9 +44,64 @@ struct session_info {
>  
>  #define INTERFACE_CK_SEAT       INTERFACE_CONSOLE_KIT ".Seat"
>  
> +#define CK_SEAT_SIGNAL_ACTIVE_SESSION_CHANGED       "ActiveSessionChanged"
> +
>  static char *console_kit_get_first_seat(struct session_info *si);
>  static char *console_kit_check_active_session_change(struct session_info
> *si);
>  
> +static void
> +si_dbus_read_signals(struct session_info *si)
> +{
> +    DBusMessage *message = NULL;
> +
> +    dbus_connection_read_write(si->connection, 0);
> +    message = dbus_connection_pop_message(si->connection);
> +    while (message != NULL) {

I think it might be a bit more readable and robust to do something like:

while ((message = dbus_connection_pop_message(si->connection)) != NULL)

That way you don't need to worry about skipping over the assignment at the very
end of this loop if you ever add a 'continue' statement to the loop. It should
work right now since you removed all of the 'continue' statements that used to
be present in the old code, but...

> +        const char *member;
> +
> +        if (dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_SIGNAL) {
> +            syslog(LOG_WARNING, "(console-kit) received non signal message");
> +            dbus_message_unref(message);
> +            break;
> +        }
> +
> +        member = dbus_message_get_member (message);
> +        if (g_strcmp0(member, CK_SEAT_SIGNAL_ACTIVE_SESSION_CHANGED) == 0) {
> +            DBusMessageIter iter;
> +            gint type;
> +            gchar *session;
> +
> +            free(si->active_session);
> +            si->active_session = NULL;
> +
> +            dbus_message_iter_init(message, &iter);
> +            type = dbus_message_iter_get_arg_type(&iter);
> +            if (type == DBUS_TYPE_STRING || type == DBUS_TYPE_OBJECT_PATH) {
> +                dbus_message_iter_get_basic(&iter, &session);
> +                if (session != NULL && session[0] != '\0') {
> +                    si->active_session = g_strdup(session);
> +                } else if (si->verbose) {
> +                    syslog(LOG_WARNING, "(console-kit) received invalid
> session. "
> +                           "No active-session at the moment");
> +                }
> +            } else {
> +                /* Session should be an object path, but there is a bug in
> +                   ConsoleKit where it sends a string rather then an
> object_path
> +                   accept object_path too in case the bug ever gets fixed */

It seems that this comment got moved to the wrong spot in the code. Should be up
above where you check 'type', I guess.

> +                syslog(LOG_ERR,
> +                       "ActiveSessionChanged message has unexpected type:
> '%c'",
> +                       type);
> +            }
> +        } else if (si->verbose) {
> +            syslog(LOG_DEBUG, "(console-kit) Signal not handled: %s",
> member);
> +        }
> +
> +        dbus_message_unref(message);
> +        dbus_connection_read_write(si->connection, 0);
> +        message = dbus_connection_pop_message(si->connection);
> +    }
> +}
> +
>  struct session_info *session_info_create(int verbose)
>  {
>      struct session_info *si;
> @@ -316,60 +372,7 @@ exit:
>  
>  static char *console_kit_check_active_session_change(struct session_info *si)
>  {
> -    DBusMessage *message = NULL;
> -    DBusMessageIter iter;
> -    char *session;
> -    int type;
> -
> -    /* non blocking read of the next available message */
> -    dbus_connection_read_write(si->connection, 0);
> -    while ((message = dbus_connection_pop_message(si->connection))) {
> -        if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_SIGNAL) {
> -            const char *member = dbus_message_get_member (message);
> -            if (!strcmp(member, "NameAcquired")) {
> -                dbus_message_unref(message);
> -                continue;
> -            }
> -            if (strcmp(member, "ActiveSessionChanged")) {
> -                syslog(LOG_ERR, "unexpected signal member: %s", member);
> -                dbus_message_unref(message);
> -                continue;
> -            }
> -        } else {
> -            syslog(LOG_ERR, "received non signal message!");
> -            dbus_message_unref(message);
> -            continue;
> -        }
> -
> -        free(si->active_session);
> -        si->active_session = NULL;
> -
> -        dbus_message_iter_init(message, &iter);
> -        type = dbus_message_iter_get_arg_type(&iter);
> -        /* Session should be an object path, but there is a bug in
> -           ConsoleKit where it sends a string rather then an object_path
> -           accept object_path too in case the bug ever gets fixed */
> -        if (type != DBUS_TYPE_STRING && type != DBUS_TYPE_OBJECT_PATH) {
> -            syslog(LOG_ERR,
> -                   "ActiveSessionChanged message has unexpected type: '%c'",
> -                   type);
> -            dbus_message_unref(message);
> -            continue;
> -        }
> -
> -        dbus_message_iter_get_basic(&iter, &session);
> -        if (session != NULL && session[0] != '\0') {
> -            si->active_session = strdup(session);
> -        } else if (si->verbose) {
> -            syslog(LOG_WARNING, "(console-kit) received invalid session. "
> -                   "No active-session at the moment");
> -        }
> -        dbus_message_unref(message);
> -
> -        /* non blocking read of the next available message */
> -        dbus_connection_read_write(si->connection, 0);
> -    }
> -
> +    si_dbus_read_signals(si);
>      if (si->verbose)
>          syslog(LOG_DEBUG, "(console-kit) active-session: '%s'",
>                 (si->active_session ? si->active_session : "None"));

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