On Fri, 2016-06-03 at 09:44 +0200, Victor Toso wrote:
> Hi,
>
> On Thu, Jun 02, 2016 at 07:43:25PM +0200, Pavel Grunt wrote:
> > Hi,
> >
> > On Fri, 2016-05-27 at 11:42 +0200, Victor Toso wrote:
> > > Property introduced in v230 of systemd.
> > >
> > > Systems that don't have up to date systemd-login will get the
> > > following log message:
> > >
> > > "Properties.Get failed: Unknown property or interface."
> > Can we give more info (which property is unknown) ? To make it easier for
> > users.
> > The property is new, so almost everyone will see the message in the log.
>
> Right, I hard-coded '(locked-hint)' string in the error message as the
> message comes from dbus-1. I hope we can get better error messages when
> using gdbus in the future.
thanks
>
> > >
> > > Resolves: rhbz#1323623
> > > ---
> > > src/systemd-login.c | 102
> > > ++++++++++++++++++++++++++++++++++++++++++++++++---
> > > -
> > > 1 file changed, 95 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/systemd-login.c b/src/systemd-login.c
> > > index 730547a..c56fca4 100644
> > > --- a/src/systemd-login.c
> > > +++ b/src/systemd-login.c
> > > @@ -36,14 +36,21 @@ struct session_info {
> > > char *match_session_signals;
> > > } dbus;
> > > gboolean session_is_locked;
> > > + gboolean session_locked_hint;
> > > };
> > >
> > > +#define LOGIND_INTERFACE "org.freedesktop.login1"
> > > +
> > > #define LOGIND_SESSION_INTERFACE "org.freedesktop.login1.Session"
> > > #define LOGIND_SESSION_OBJ_TEMPLATE
> > > "/org/freedesktop/login1/session/_3%s"
> > >
> > > +#define DBUS_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties"
> > > +
> > > #define SESSION_SIGNAL_LOCK "Lock"
> > > #define SESSION_SIGNAL_UNLOCK "Unlock"
> > >
> > > +#define SESSION_PROP_LOCKED_HINT "LockedHint"
> > > +
> > > /* dbus related */
> > > static DBusConnection *si_dbus_get_system_bus(void)
> > > {
> > > @@ -112,6 +119,84 @@ static void si_dbus_match_rule_update(struct
> > > session_info
> > > *si)
> > > }
> > >
> > > static void
> > > +si_dbus_read_properties(struct session_info *si)
> > > +{
> > > + dbus_bool_t locked_hint, ret;
> > > + DBusMessageIter iter, iter_variant;
> > > + gint type;
> > > + DBusError error;
> > > + DBusMessage *message = NULL;
> > > + DBusMessage *reply = NULL;
> > > + gchar *session_object;
> > > + const gchar *interface, *property;
> > > +
> > > + if (si->session == NULL)
> > > + return;
> > > +
> > > + session_object = g_strdup_printf(LOGIND_SESSION_OBJ_TEMPLATE, si-
> > > > session);
> > > + message = dbus_message_new_method_call(LOGIND_INTERFACE,
> > > + session_object,
> > > + DBUS_PROPERTIES_INTERFACE,
> > > + "Get");
> > > + g_free (session_object);
> > > + if (message == NULL) {
> > > + syslog(LOG_ERR, "Unable to create dbus message");
> > > + goto exit;
> > > + }
> > > +
> > > + interface = LOGIND_SESSION_INTERFACE;
> > > + property = SESSION_PROP_LOCKED_HINT;
> > > + ret = dbus_message_append_args(message,
> > > + DBUS_TYPE_STRING, &interface,
> > > + DBUS_TYPE_STRING, &property,
> > > + DBUS_TYPE_INVALID);
> > > + if (!ret) {
> > > + syslog(LOG_ERR, "Unable to request locked-hint");
> > > + goto exit;
> > > + }
> > > +
> > > + dbus_error_init(&error);
> > > + reply = dbus_connection_send_with_reply_and_block(si-
> > > > dbus.system_connection,
> > > + message,
> > > + -1,
> > > + &error);
> > > + if (reply == NULL || dbus_error_is_set(&error)) {
> > According to function definition error is set on failure, so this block can
> > be
> > simplified
>
> I'm following the same approach Hans did with console-kit so I would
> rather to keep it this way till we move to gdbus. I hope that gdbus
> would improve the error handling and not let things like [0] happen for
> instance.
>
> [0] https://cgit.freedesktop.org/spice/linux/vd_agent/tree/src/console-kit.c#n
> 170
>
> Also, I was taught [1] that checking the return value of the function is
> better then checking the error.
Definitely, the function returns NULL w/o setting error when validating its
parameters. But the reply cannot be nonnull and error set. I just wanted to say
that the second part of the condition is redundant: 'if (reply == NULL)' is
enough
Pavel
>
> [1] https://bugzilla.gnome.org/show_bug.cgi?id=763046#c20
>
> > > + if (dbus_error_is_set(&error)) {
> > > + syslog(LOG_ERR, "Properties.Get failed: %s", error.message);
> > > + dbus_error_free(&error);
> > > + } else {
> > > + syslog(LOG_ERR, "Properties.Get failed");
> > > + }
> > > + goto exit;
> > > + }
> > > +
> > > + dbus_message_iter_init(reply, &iter);
> > > + type = dbus_message_iter_get_arg_type(&iter);
> > > + if (type != DBUS_TYPE_VARIANT) {
> > > + syslog(LOG_ERR, "expected a variant, got a '%c' instead", type);
> > > + goto exit;
> > > + }
> > > +
> > > + dbus_message_iter_recurse(&iter, &iter_variant);
> > > + type = dbus_message_iter_get_arg_type(&iter_variant);
> > > + if (type != DBUS_TYPE_BOOLEAN) {
> > > + syslog(LOG_ERR, "expected a boolean, got a '%c' instead", type);
> > > + goto exit;
> > > + }
> > > + dbus_message_iter_get_basic(&iter_variant, &locked_hint);
> > > +
> > > + si->session_locked_hint = (locked_hint) ? TRUE : FALSE;
> > > +exit:
> > > + if (reply != NULL) {
> > > + dbus_message_unref(reply);
> > > + }
> > > +
> > > + if (message != NULL) {
> > > + dbus_message_unref(message);
> > > + }
> > > +}
> > > +
> > > +static void
> > > si_dbus_read_signals(struct session_info *si)
> > > {
> > > DBusMessage *message = NULL;
> > > @@ -220,16 +305,19 @@ char *session_info_session_for_pid(struct
> > > session_info
> > > *si, uint32_t pid)
> > >
> > > gboolean session_info_session_is_locked(struct session_info *si)
> > > {
> > > - g_return_val_if_fail (si != NULL, FALSE);
> > > + gboolean locked;
> > >
> > > - /* 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 */
> > > + g_return_val_if_fail (si != NULL, FALSE);
> > >
> > > si_dbus_read_signals(si);
> > > - return si->session_is_locked;
> > > + si_dbus_read_properties(si);
> > > +
> > > + locked = (si->session_is_locked || si->session_locked_hint);
> > > + if (si->verbose) {
> > > + syslog(LOG_DEBUG, "(systemd-login) session is locked: %s",
> > > + locked ? "yes" : "no");
> > > + }
> > > + return locked;
> > > }
> > >
> > > /* This function should only be called after
> > > session_info_get_active_session
> >
> > Looks good.
> >
> > Thanks,
> > Pavel
>
> Thanks for reviewing, I'll be sending a v2 shortly.
>
> Cheers,
> toso
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel