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?
>
> > + 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