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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2016-04-28 at 13:38 +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, Apr 27, 2016 at 03:07:52PM -0500, Jonathon Jongsma wrote:
> > 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...
> 
> Main reason for the change is that, if no continue is hit, we do
> "dbus_connection_read_write" before
> "dbus_connection_pop_message(si->connection);" but if a continue is hit, we
> miss
> the _read_write. I thought it could be problematic and for that reason I
> rearrange the code. What do you think about it? Should we move it back as it
> was?


Well, I was not arguing that we should go back to using the 'continue'
statements. I think your changes in that area are fine (and probably an
improvment considering the point you mention above). I was only trying to
illustrate that if we *did* add a 'continue' at some point in the future, we
would end up skipping over the pop_message() call (similar to how the old code
skipped over the read_write() call). Putting the read_message() call within the
loop condition check avoids this possibility and is therefore a bit more robust.
But I don't really care too much.

> 
> > 
> > > +        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.
> 
> True, I'll fix it. Thanks!
> 
> > > +                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@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]