[Spice-devel] [vdagent-linux v3 1/4] session-info: check for a locked session
Victor Toso
lists at victortoso.com
Sat Apr 30 06:06:14 UTC 2016
Hi,
On Fri, Apr 29, 2016 at 09:59:03AM -0500, Jonathon Jongsma wrote:
> 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.
Thanks, I'll fix it in v4.
> (probably this was just copied from the consolekit implementation,
> which means that one should probably be fixed eventually as well)
Hehe, actually it was the other way around, I've started with systemd
integration. I fixed a few of these pointers there in the clean-up
series but I can see I forgot this one there at least. I'll try to
address them soon too.
>
> > + }
> > +}
> > +
> > +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.
I thought about ignoring and it should be fine indeed. My main concern
was that, if we are receiving something that we did not request to,
it is probably worth it to handle as a failure.
As I don't think this really happens, we can probably ignore it and move
to the next message.
I'll change it.
>
> > +
> > + 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.
Indeed. I'll change so we can send back VD_AGENT_FILE_XFER_STATUS_ERROR
instead of VD_AGENT_FILE_XFER_STATUS_CANCELLED.
toso
>
>
> > + return;
> > }
> > udscs_write(active_session_conn, VDAGENTD_FILE_XFER_START, 0, 0,
> > data, message_header->size);
>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list