On Thu, 2016-04-28 at 15:37 +0200, Victor Toso wrote: > Each session back-end can return this information to vdagentd when > requested. > > The agent should use this on situations that should not work when > session is locked such as file-transfer-start which is fixed by this > patch. > > systemd-login is the only back-end implementing this function at the > moment and I'll address console-kit back-end in a later patch. > > Also, this patch makes spice-vdagent depend on dbus for getting the > lock information. > > Resolve: https://bugzilla.redhat.com/show_bug.cgi?id=1323623 > --- > configure.ac | 17 ++----- > src/console-kit.c | 7 +++ > src/dummy-session-info.c | 5 ++ > src/session-info.h | 3 ++ > src/systemd-login.c | 128 > +++++++++++++++++++++++++++++++++++++++++++++++ > src/vdagentd.c | 7 +++ > 6 files changed, 154 insertions(+), 13 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 2fe685b..fc5943b 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -81,6 +81,7 @@ PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.28]) > PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11]) > PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.8]) > PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22]) > +PKG_CHECK_MODULES([DBUS], [dbus-1]) > > if test "$with_session_info" = "auto" || test "$with_session_info" = > "systemd"; then > PKG_CHECK_MODULES([LIBSYSTEMD_LOGIN], > @@ -107,19 +108,9 @@ fi > AM_CONDITIONAL(HAVE_LIBSYSTEMD_LOGIN, test x"$have_libsystemd_login" = > "xyes") > > if test "$with_session_info" = "auto" || test "$with_session_info" = "console > -kit"; then > - PKG_CHECK_MODULES([DBUS], > - [dbus-1], > - [have_console_kit="yes"], > - [have_console_kit="no"]) > - if test x"$have_console_kit" = "xno" && test "$with_session_info" = > "console-kit"; then > - AC_MSG_ERROR([console-kit support explicitly requested, but some > required packages are not available]) > - fi > - if test x"$have_console_kit" = "xyes"; then > - AC_DEFINE([HAVE_CONSOLE_KIT], [1], [If defined, vdagentd will be > compiled with ConsoleKit support]) > - with_session_info="console-kit" > - else > - with_session_info="none" > - fi > + AC_DEFINE([HAVE_CONSOLE_KIT], [1], [If defined, vdagentd will be compiled > with ConsoleKit support]) > + have_console_kit="yes" > + with_session_info="console-kit" > else > have_console_kit="no" > fi > diff --git a/src/console-kit.c b/src/console-kit.c > index d4eecd7..72a5b16 100644 > --- a/src/console-kit.c > +++ b/src/console-kit.c > @@ -416,3 +416,10 @@ static char > *console_kit_check_active_session_change(struct session_info *info) > > return info->active_session; > } > + > +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; > +} > diff --git a/src/dummy-session-info.c b/src/dummy-session-info.c > index 2f0ae65..e7a716f 100644 > --- a/src/dummy-session-info.c > +++ b/src/dummy-session-info.c > @@ -44,3 +44,8 @@ char *session_info_session_for_pid(struct session_info *si, > uint32_t pid) > { > return NULL; > } > + > +gboolean session_is_locked(struct session_info *ck) > +{ > + return FALSE; > +} > diff --git a/src/session-info.h b/src/session-info.h > index c4f8187..f395dbf 100644 > --- a/src/session-info.h > +++ b/src/session-info.h > @@ -24,6 +24,7 @@ > > #include <stdio.h> > #include <stdint.h> > +#include <glib.h> > > struct session_info; > > @@ -36,4 +37,6 @@ const char *session_info_get_active_session(struct > session_info *ck); > /* Note result must be free()-ed by caller */ > char *session_info_session_for_pid(struct session_info *ck, uint32_t pid); > > +gboolean session_info_session_is_locked(struct session_info *si); > + > #endif > diff --git a/src/systemd-login.c b/src/systemd-login.c > index 9ba300a..470a9ff 100644 > --- a/src/systemd-login.c > +++ b/src/systemd-login.c > @@ -25,13 +25,121 @@ > #include <string.h> > #include <syslog.h> > #include <systemd/sd-login.h> > +#include <dbus/dbus.h> > > struct session_info { > int verbose; > sd_login_monitor *mon; > char *session; > + struct { > + DBusConnection *system_connection; > + char *match_session_signals; > + } dbus; > + gboolean session_is_locked; > }; > > +#define LOGIND_SESSION_INTERFACE "org.freedesktop.login1.Session" > +#define LOGIND_SESSION_OBJ_TEMPLATE "'/org/freedesktop/login1/session/_3%s'" > + > +#define SESSION_SIGNAL_LOCK "Lock" > +#define SESSION_SIGNAL_UNLOCK "Unlock" > + > +/* dbus related */ > +static DBusConnection *si_dbus_get_system_bus(void) > +{ > + DBusConnection *connection; > + DBusError error; > + > + dbus_error_init(&error); > + connection = dbus_bus_get_private(DBUS_BUS_SYSTEM, &error); > + if (connection == NULL || dbus_error_is_set(&error)) { > + if (dbus_error_is_set(&error)) { > + syslog(LOG_WARNING, "Unable to connect to system bus: %s", > + error.message); > + dbus_error_free(&error); > + } else { > + syslog(LOG_WARNING, "Unable to connect to system bus"); > + } > + return NULL; > + } > + return connection; > +} > + > +static void si_dbus_match_remove(struct session_info *si) > +{ > + DBusError error; > + if (si->dbus.match_session_signals == NULL) > + return; > + > + dbus_error_init(&error); > + dbus_bus_remove_match(si->dbus.system_connection, > + si->dbus.match_session_signals, > + &error); > + > + g_free(si->dbus.match_session_signals); > +} > + > +static void si_dbus_match_rule_update(struct session_info *si) > +{ > + DBusError error; > + > + if (si->dbus.system_connection == NULL || > + si->session == NULL) > + return; > + > + si_dbus_match_remove(si); > + > + si->dbus.match_session_signals = > + g_strdup_printf ("type='signal',interface='%s',path=" > + LOGIND_SESSION_OBJ_TEMPLATE, > + LOGIND_SESSION_INTERFACE, > + si->session); > + if (si->verbose) > + syslog(LOG_DEBUG, "logind match: %s", si > ->dbus.match_session_signals); > + > + dbus_error_init(&error); > + dbus_bus_add_match(si->dbus.system_connection, > + si->dbus.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(si->dbus.match_session_signals); dangling pointer here. Set it to NULL. (probably this was just copied from the consolekit implementation, which means that one should probably be fixed eventually as well) > + } > +} > + > +static void > +si_dbus_read_signals(struct session_info *si) > +{ > + DBusMessage *message = NULL; > + > + dbus_connection_read_write(si->dbus.system_connection, 0); > + message = dbus_connection_pop_message(si->dbus.system_connection); > + while (message != NULL) { > + const char *member; > + > + if (dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_SIGNAL) { > + syslog(LOG_WARNING, "(systemd-login) received non signal > message"); > + dbus_message_unref(message); > + break; > + } I didn't really notice this when reviewing the console-kit implementation, but what is the purpose of using 'break' here? I would have thought that if we encountered an unexpected message, we'd want to just ignore it and continue reading the remaining messages rather than aborting the loop and returning. > + > + member = dbus_message_get_member (message); > + if (g_strcmp0(member, SESSION_SIGNAL_LOCK) == 0) { > + si->session_is_locked = TRUE; > + } else if (g_strcmp0(member, SESSION_SIGNAL_UNLOCK) == 0) { > + si->session_is_locked = FALSE; > + } else if (si->verbose) { > + syslog(LOG_DEBUG, "(systemd-login) Signal not handled: %s", > member); > + } > + > + dbus_message_unref(message); > + dbus_connection_read_write(si->dbus.system_connection, 0); > + message = dbus_connection_pop_message(si->dbus.system_connection); > + } > +} > + > struct session_info *session_info_create(int verbose) > { > struct session_info *si; > @@ -42,6 +150,7 @@ struct session_info *session_info_create(int verbose) > return NULL; > > si->verbose = verbose; > + si->session_is_locked = FALSE; > > r = sd_login_monitor_new("session", &si->mon); > if (r < 0) { > @@ -50,6 +159,7 @@ struct session_info *session_info_create(int verbose) > return NULL; > } > > + si->dbus.system_connection = si_dbus_get_system_bus(); > return si; > } > > @@ -58,6 +168,8 @@ void session_info_destroy(struct session_info *si) > if (!si) > return; > > + si_dbus_match_remove(si); > + dbus_connection_close(si->dbus.system_connection); > sd_login_monitor_unref(si->mon); > free(si->session); > free(si); > @@ -87,6 +199,7 @@ const char *session_info_get_active_session(struct > session_info *si) > sd_login_monitor_flush(si->mon); > free(old_session); > > + si_dbus_match_rule_update(si); > return si->session; > } > > @@ -104,3 +217,18 @@ char *session_info_session_for_pid(struct session_info > *si, uint32_t pid) > > return session; > } > + > +gboolean session_info_session_is_locked(struct session_info *si) > +{ > + if (si == NULL) > + return FALSE; > + > + /* We could also rely on IdleHint property from Session which seems to > work > + * well in rhel7 but it wasn't working well in my own system (F23). I'm > + * convinced for now that Lock/Unlock signals should be enough but that > + * means Lock/Unlock being done by logind. That might take a while. > + * Check: https://bugzilla.gnome.org/show_bug.cgi?id=764773 */ > + > + si_dbus_read_signals(si); > + return si->session_is_locked; > +} > diff --git a/src/vdagentd.c b/src/vdagentd.c > index 69332ff..efcf951 100644 > --- a/src/vdagentd.c > +++ b/src/vdagentd.c > @@ -279,6 +279,13 @@ static void do_client_file_xfer(struct > vdagent_virtio_port *vport, > "active session, cancelling client file-xfer request %u", > s->id); > return; > + } else if (session_info_session_is_locked(session_info)) { > + syslog(LOG_DEBUG, "Session is locked, skipping file-xfer-start"); > + cancel_file_xfer(vport, > + "User's session is locked and cannot start file transfer. " > + "Cancelling client file-xfer request %u", > + s->id); Wouldn't it be better to return an error than to cancel the file transfer? To me, canceling implies some sort of user action to cancel the operation. Whereas an error implies a condition that could not be met. This scenario seems to be a better match for the latter. > + return; > } > udscs_write(active_session_conn, VDAGENTD_FILE_XFER_START, 0, 0, > data, message_header->size); Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel