[systemd-devel] [PATCH v2 05/10] logind: introduce session-devices

[Lennart Poettering]

On Tue, 17.09.13 17:39, David Herrmann (dh.herrmann at gmail.com) wrote:

>          "   <arg name=\"force\" type=\"b\"/>\n"                         \
>          "  </method>\n"                                                 \
>          "  <method name=\"DropControl\"/>\n"                            \
> +        "  <method name=\"RequestDevice\">\n"                           \
> +        "   <arg name=\"node\" type=\"s\" direction=\"in\"/>\n"         \
> +        "   <arg name=\"fd\" type=\"h\" direction=\"out\"/>\n"          \
> +        "   <arg name=\"paused\" type=\"b\" direction=\"out\"/>\n"      \
> +        "  </method>\n"                                                 \
> +        "  <method name=\"ReleaseDevice\">\n"                           \
> +        "   <arg name=\"node\" type=\"s\"/>\n"                          \
> +        "  </method>\n"
>          \

Please rename this pair to TakeDevice() and ReleaseDevice().

> index 0000000..659f161
> --- /dev/null
> +++ b/src/login/logind-session-device.c
> @@ -0,0 +1,489 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
> +/***
> +  This file is part of systemd.
> +
> +  Copyright 2013 David Herrmann
> +
> +  systemd is free software; you can redistribute it and/or modify it
> +  under the terms of the GNU Lesser General Public License as published by
> +  the Free Software Foundation; either version 2.1 of the License, or
> +  (at your option) any later version.
> +
> +  systemd is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  Lesser General Public License for more details.
> +
> +  You should have received a copy of the GNU Lesser General Public License
> +  along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include <assert.h>
> +#include <fcntl.h>
> +#include <libudev.h>
> +#include <linux/input.h>
> +#include <linux/ioctl.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "dbus-common.h"
> +#include "logind-session-device.h"
> +#include "util.h"
> +
> +enum SessionDeviceNotifications {
> +        SESSION_DEVICE_RESUME,
> +        SESSION_DEVICE_TRY_PAUSE,
> +        SESSION_DEVICE_PAUSE,
> +        SESSION_DEVICE_RELEASE,
> +};
> +
> +static void session_device_notify(SessionDevice *sd, enum SessionDeviceNotifications type) {
> +        _cleanup_dbus_message_unref_ DBusMessage *m = NULL;
> +        _cleanup_free_ char *path = NULL;
> +        const char *t = NULL;
> +
> +        assert(sd);
> +
> +        if (!sd->session->controller)
> +                return;
> +
> +        path = session_bus_path(sd->session);
> +        if (!path)
> +                return;
> +
> +        m = dbus_message_new_signal(path,
> +                                    "org.freedesktop.login1.Session",
> +                                    (type == SESSION_DEVICE_RESUME) ? "ResumeDevice" : "PauseDevice");
> +        if (!m)
> +                return;
> +
> +        if (!dbus_message_set_destination(m, sd->session->controller))
> +                return;
> +
> +        switch (type) {
> +        case SESSION_DEVICE_RESUME:
> +                if (!dbus_message_append_args(m,
> +                                              DBUS_TYPE_STRING, &sd->node,
> +                                              DBUS_TYPE_UNIX_FD, &sd->fd,
> +                                              DBUS_TYPE_INVALID))
> +                        return;
> +                break;
> +        case SESSION_DEVICE_TRY_PAUSE:
> +                t = "pause";
> +                break;
> +        case SESSION_DEVICE_PAUSE:
> +                t = "force";
> +                break;
> +        case SESSION_DEVICE_RELEASE:
> +                t = "gone";
> +                break;
> +        default:
> +                return;
> +        }
> +
> +        if (t && !dbus_message_append_args(m,
> +                                           DBUS_TYPE_STRING, &sd->node,
> +                                           DBUS_TYPE_STRING, &t,
> +                                           DBUS_TYPE_INVALID))
> +                return;
> +
> +        dbus_connection_send(sd->session->manager->bus, m, NULL);
> +}
> +
> +static int sd_eviocrevoke(int fd)
> +{

"{" please on the same line as the function name.

> +        static bool warned;
> +        int r;
> +
> +#ifndef EVIOCREVOKE
> +#  define EVIOCREVOKE _IOW('E', 0x91, int)
> +#endif

Please move this to missing.h.

> +
> +        assert(fd >= 0);
> +
> +        r = ioctl(fd, EVIOCREVOKE, 1);
> +        if (r < 0) {
> +                r = -errno;
> +                if (r == -ENOSYS && !warned) {
> +                        warned = true;
> +                        log_warning("kernel does not support evdev-revocation");
> +                }
> +        }
> +
> +        return 0;
> +}
> +
> +static int sd_drmsetmaster(int fd)
> +{

"{"...

> +        int r;
> +
> +#ifndef DRM_IOCTL_SET_MASTER
> +#  define DRM_IOCTL_SET_MASTER _IO('d', 0x1e)
> +#endif

→ missing.h!

> +
> +        assert(fd >= 0);
> +
> +        r = ioctl(fd, DRM_IOCTL_SET_MASTER, 0);
> +        if (r < 0)
> +                return -errno;
> +
> +        return 0;
> +}
> +
> +static int sd_drmdropmaster(int fd)
> +{

"{"....

> +        int r;
> +
> +#ifndef DRM_IOCTL_DROP_MASTER
> +#  define DRM_IOCTL_DROP_MASTER _IO('d', 0x1f)
> +#endif

→ missing.h

> +        switch (sd->type) {
> +        case DEVICE_TYPE_DRM:
> +                if (active) {
> +                        sd_drmsetmaster(fd);
> +                } else {

Please remove "{}".


> +        dnum = udev_device_get_devnum(dev);
> +        sysname = udev_device_get_sysname(dev);
> +        type = DEVICE_TYPE_UNKNOWN;
> +
> +        if (major(dnum) == 29) {
> +                if (startswith(sysname, "fb"))
> +                        type = DEVICE_TYPE_FBDEV;
> +        } else if (major(dnum) == 226) {
> +                if (startswith(sysname, "card"))
> +                        type = DEVICE_TYPE_DRM;
> +        } else if (major(dnum) == 13) {
> +                if (startswith(sysname, "event"))
> +                        type = DEVICE_TYPE_EVDEV;

Please use udev_device_get_subsystem() for this! Comparing majors is evil!

> +        dev = udev_device_new_from_devnum(sd->session->manager->udev, 'c', st.st_rdev);
> +        if (!dev)
> +                return -ENODEV;
> +        sp = udev_device_get_syspath(dev);
> +
> +        /* Only allow access to "uaccess" tagged devices! */
> +        if (!udev_device_has_tag(dev, "uaccess")) {
> +                r = -EPERM;
> +                goto err_dev;
> +        }
> +

This sounds wrong, after all the suer could access the device directly
anyway if uaccess is set.

> +        /* Only allow opening the _real_ device node. This is the node provided
> +         * by devtmpfs! Issue a readlink() if you are not sure. We cannot allow
> +         * any path here as user-space could otherwise trigger OOM by requesting
> +         * duplicates. */
> +        real_node = udev_device_get_devnode(dev);
> +        if (!streq_ptr(real_node, node)) {
> +                r = -EINVAL;
> +                goto err_dev;
> +        }

I think it would make sense to drop this. Instead verify with
path_startswith() that the specified path is in /dev. Then do an lstat()
on the specified device path, and verify the backing major/minor matches
the one of /dev, so that people cannot use /dev/shm to confuse us.

> +typedef struct SessionDevice SessionDevice;
> +
> +#include "list.h"
> +#include "util.h"
> +#include "logind.h"
> +#include "logind-device.h"
> +#include "logind-seat.h"
> +#include "logind-session.h"
> +
> +enum DeviceType {
> +        DEVICE_TYPE_UNKNOWN,
> +        DEVICE_TYPE_FBDEV,
> +        DEVICE_TYPE_DRM,
> +        DEVICE_TYPE_EVDEV,
> +};

exported enums plese with a typdef!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.