[PATCH 08/12] systemd-logind: Add systemd-logind "core"
Hans de Goede
hdegoede at redhat.com
Tue Jan 28 01:30:22 PST 2014
Hi,
On 01/28/2014 08:15 AM, Peter Hutterer wrote:
> On Wed, Jan 15, 2014 at 03:32:22PM +0100, Hans de Goede wrote:
>> This commits add the bulk of the systemd-logind integration code, but does
>> not hook it up yet other then calling its init and fini functions, which
>> don't do that much.
<snip>
>> @@ -902,6 +903,26 @@ if test "x$CONFIG_HAL" = xyes; then
>> fi
>> AM_CONDITIONAL(CONFIG_HAL, [test "x$CONFIG_HAL" = xyes])
>>
>> +if test "x$SYSTEMD_LOGIND" = xauto; then
>> + if test "x$HAVE_DBUS" = xyes -a "x$CONFIG_UDEV" = xyes ; then
>> + SYSTEMD_LOGIND=yes
>> + else
>> + SYSTEMD_LOGIND=no
>> + fi
>> +fi
>> +if test "x$SYSTEMD_LOGIND" = xyes; then
>> + if ! test "x$HAVE_DBUS" = xyes; then
>> + AC_MSG_ERROR([systemd-logind requested, but D-Bus is not installed.])
>> + fi
>> + if ! test "x$CONFIG_UDEV" = xyes ; then
>> + AC_MSG_ERROR([systemd-logind is only supported in combination with udev configuration.])
>> + fi
>> +
>> + AC_DEFINE(SYSTEMD_LOGIND, 1, [Enable systemd-logind integration])
>> + NEED_DBUS="yes"
>> +fi
>> +AM_CONDITIONAL(SYSTEMD_LOGIND, [test "x$SYSTEMD_LOGIND" = xyes])
>> +
>
> this looks a bit odd - don't we need to check for some systemd-specific bits as
> well here? if not, or if we're already checking for it, maybe note that in
> the commit message.
We only use dbus to talk to systemd-logind, so no other libs are needed. I've amended
the commit message to reflect this.
>
>> if test "x$NEED_DBUS" = xyes; then
>> AC_DEFINE(NEED_DBUS, 1, [Enable D-Bus core])
>> fi
>> diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
>> index 815f679..380a70e 100644
>> --- a/hw/xfree86/common/xf86Init.c
>> +++ b/hw/xfree86/common/xf86Init.c
>> @@ -54,6 +54,7 @@
>> #include "site.h"
>> #include "mi.h"
>> #include "dbus-core.h"
>> +#include "systemd-logind.h"
>>
>> #include "compiler.h"
>>
>> @@ -458,6 +459,7 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv)
>> DoShowOptions();
>>
>> dbus_core_init();
>> + systemd_logind_init();
>
> just making sure: we won't have a namespace clash with a current or furture
> systemd_logind_*? could always go for the tried and tested xfoo naming
> convention (xsystemd_logind_...)
See David Herrmann's answer.
<snip>
>> + if (!info->session || devnum == 0)
>> + return -1;
>> +
>> + /* logind does not support mouse devs (with evdev we don't need them) */
>> + if (strstr(path, "mouse"))
>> + return -1;
>
> hmm, is there some better way of checking this rather than a string
> comparison on the path?
I'm afraid not.
>
>> +
>> + dbus_error_init(&error);
>> +
>> + msg = dbus_message_new_method_call("org.freedesktop.login1", info->session,
>> + "org.freedesktop.login1.Session", "TakeDevice");
>> + if (!msg) {
>> + LogMessage(X_ERROR, "systemd-logind: out of memory\n");
>> + goto cleanup;
>> + }
>> +
>> + if (!dbus_message_append_args(msg, DBUS_TYPE_UINT32, &major,
>> + DBUS_TYPE_UINT32, &minor,
>> + DBUS_TYPE_INVALID)) {
>> + LogMessage(X_ERROR, "systemd-logind: out of memory\n");
>> + goto cleanup;
>> + }
>> +
>> + reply = dbus_connection_send_with_reply_and_block(info->conn, msg,
>> + DBUS_TIMEOUT, &error);
>> + if (!reply) {
>> + LogMessage(X_ERROR, "systemd-logind: failed to take device %s: %s\n",
>> + path, error.message);
>> + goto cleanup;
>> + }
>> +
>> + if (!dbus_message_get_args(reply, &error,
>> + DBUS_TYPE_UNIX_FD, &fd,
>> + DBUS_TYPE_BOOLEAN, &paused,
>> + DBUS_TYPE_INVALID)) {
>> + LogMessage(X_ERROR, "systemd-logind: TakeDevice %s: %s\n",
>> + path, error.message);
>> + goto cleanup;
>> + }
>> +
>> + *paused_ret = paused;
>> +
>> + LogMessage(X_INFO, "systemd-logind: got fd for %s %u:%u fd %d paused %d\n",
>> + path, major, minor, fd, paused);
>> +
>> +cleanup:
>> + if (msg)
>> + dbus_message_unref(msg);
>> + if (reply)
>> + dbus_message_unref(reply);
>> + dbus_error_free(&error);
>> +
>> + return fd;
>> +}
>> +
>> +void
>> +systemd_logind_put_fd(dev_t devnum)
>
> I find it funny that you use get/put fd when the logind API itself calls it
> "take" and "release". maybe we should stick close to that naming?
> or get_fd and release_fd or so, I found put_fd a bit confusing.
Fixed in my local tree.
<snip>
>> +static void
>> +connect_hook(DBusConnection *connection, void *data)
>> +{
>> + struct systemd_logind_info *info = data;
>> + DBusError error;
>> + DBusMessage *msg = NULL;
>> + DBusMessage *reply = NULL;
>> + dbus_int32_t arg;
>> + char *session = NULL;
>> +
>> + dbus_error_init(&error);
>> +
>> + msg = dbus_message_new_method_call("org.freedesktop.login1",
>> + "/org/freedesktop/login1", "org.freedesktop.login1.Manager",
>> + "GetSessionByPID");
>> + if (!msg) {
>> + LogMessage(X_ERROR, "systemd-logind: out of memory\n");
>> + goto cleanup;
>> + }
>> +
>> + arg = getpid();
>> + if (!dbus_message_append_args(msg, DBUS_TYPE_UINT32, &arg,
>> + DBUS_TYPE_INVALID)) {
>> + LogMessage(X_ERROR, "systemd-logind: out of memory\n");
>> + goto cleanup;
>> + }
>> +
>> + reply = dbus_connection_send_with_reply_and_block(connection, msg,
>> + DBUS_TIMEOUT, &error);
>> + if (!reply) {
>> + LogMessage(X_ERROR, "systemd-logind: failed to get session: %s\n",
>> + error.message);
>> + goto cleanup;
>> + }
>> + dbus_message_unref(msg);
>> +
>> + if (!dbus_message_get_args(reply, &error, DBUS_TYPE_OBJECT_PATH, &session,
>> + DBUS_TYPE_INVALID)) {
>> + LogMessage(X_ERROR, "systemd-logind: GetSessionByPID: %s\n",
>> + error.message);
>> + goto cleanup;
>> + }
>> + session = XNFstrdup(session);
>
> I'm curious why you have all the errors above but here you're just briging
> down the server if it fails.
Because the other error paths are somewhat expected to trigger (ie compiled
with systemd-logind support, but not using systemd-logind). Where as this call
failing means were OOM, and if we're OOM things will come crashing down sooner
or later anyways.
>
>> +
>> + dbus_message_unref(reply);
>> + reply = NULL;
>> +
>> +
>> + msg = dbus_message_new_method_call("org.freedesktop.login1",
>> + session, "org.freedesktop.login1.Session", "TakeControl");
>> + if (!msg) {
>> + LogMessage(X_ERROR, "systemd-logind: out of memory\n");
>> + goto cleanup;
>> + }
>> +
>> + arg = FALSE;
>> + if (!dbus_message_append_args(msg, DBUS_TYPE_BOOLEAN, &arg,
>> + DBUS_TYPE_INVALID)) {
>> + LogMessage(X_ERROR, "systemd-logind: out of memory\n");
>> + goto cleanup;
>> + }
>> +
>> + reply = dbus_connection_send_with_reply_and_block(connection, msg,
>> + DBUS_TIMEOUT, &error);
>> + if (!reply) {
>> + LogMessage(X_ERROR, "systemd-logind: TakeControl failed: %s\n",
>> + error.message);
>> + goto cleanup;
>> + }
>> +
>> + /*
>> + * HdG: This is not useful with systemd <= 208 since the signal only
>> + * contains invalidated property names there, rather then property, val
>
> typo: "than"
Fixed in my local tree.
Regards,
Hans
More information about the xorg-devel
mailing list