[systemd-devel] [RFC 08/12] gfx: add monitor

[Lennart Poettering]

On Wed, 27.11.13 19:48, David Herrmann (dh.herrmann at gmail.com) wrote:

> While all previous sd-gfx interfaces are self-contained and can be used
> directly on selected devices, this adds an interface to connect them all
> together. The sd_gfx_monitor can be used to monitor a session for device
> hotplugging and other device events. It is optional but is supposed to be
> the foundation of all systemd-helpers that use sd-gfx.
> 
> The main function of sd_gfx_monitor is to watch the system for udev-events
> of gpus and keyboards. For each of them, an sd_gfx_card or sd_gfx_kbd
> device is created and advertised to the application. Furthermore,
> systemd-localed integration is provided so keymap changes are immediately
> noticed and applied to active sd_gfx_kbd devices.
> 
> An sd_gfx_monitor can run in two modes:
>  - system mode: In system mode, no dbus, no logind and localed are assumed
>                 to be running and seat-information is ignored. This mode
>                 allows to run applications in initrds or emergency
>                 situations. It simply takes all devices it can find and
>                 tries to use them. However, this obviously requires to be
>                 root, otherwise, devices cannot be accessed.
>  - session mode: In session mode, the monitor assumes to be running in an
>                  logind session. If not, it returns an error. The monitor
>                  will call TakeControl on the active session and get
>                  device-access via logind. Only devices attached to the
>                  session will be used and no you're not required to be
>                  root. The caller is responsible of setting up the session
>                  before spawning the monitor.
> 
> Note that monitor setup is a blocking call as it is usually called during
> application setup (and making that async would have no gain). But at
> runtime, the monitor runs all dbus-calls and other calls asynchronously.
> 
> The sd_gfx_monitor interface is designed for fallbacks and basic system
> applications. It does not allow per-device configurations or any advanced
> eye-candy. It is trimmed for usability and simplicity, and it is optimized
> for fallback/emergency situations. Thus, the monitor provides some basic
> configuration options via the kernel command-line. systemd.gpus= allows to
> filter the GPUs to be used (by default, *all* connected GPUs are used
> together). systemd.keymap= allows to change the keymap in case localed is
> configured incorrectly.
> 
> The sd_gfx_monitor interfaces has the nice side-effect that all
> applications using it will share the same configuration. They will have
> the same monitor-setup, the same keymap setup and use the same devices. So
> if you system-boot fails, you can set systemd.XY="" to boot with a working
> configuration and all systemd-internal applications will just work.
> 
> If we ever export sd-gfx, most users probably want more configurability
> (like per-device keymaps) and thus will not use sd_gfx_monitor. However,
> for all fallbacks, it is the perfect base.

Hmm, this bit sounds a bit too high-level for including it in a library,
no? I mean, we can certainly share this bit inside of systemd, but this
sounds too specialized to ever become a public lib, no?

> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_UNIX_FD, &fd);
> +        if (r < 0)
> +                goto error;
> +
> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_BOOLEAN, &paused);
> +        if (r < 0)
> +                goto error;

Why in two steps? sd_bus_message_read(m, "hb", &fd, &paused) should work too?

> +static int gfx_logind_resume_fn(sd_bus *bus, sd_bus_message *m, void *data, sd_bus_error *err) {
> +        sd_gfx_monitor *mon = data;
> +        gfx_device *dev;
> +        int r, fd;
> +        uint32_t major, minor;
> +        dev_t devt;
> +
> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_UINT32, &major);
> +        if (r < 0)
> +                goto error;
> +
> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_UINT32, &minor);
> +        if (r < 0)
> +                goto error;
> +
> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_UNIX_FD, &fd);
> +        if (r < 0)
> +                goto error;

Same here... One call should suffice...

> +static int gfx_logind_pause_fn(sd_bus *bus, sd_bus_message *m, void *data, sd_bus_error *err) {
> +        sd_gfx_monitor *mon = data;
> +        gfx_device *dev;
> +        int r;
> +        const char *type;
> +        uint32_t major, minor;
> +        dev_t devt;
> +
> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_UINT32, &major);
> +        if (r < 0)
> +                goto error;
> +
> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_UINT32, &minor);
> +        if (r < 0)
> +                goto error;
> +
> +        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_STRING, &type);
> +        if (r < 0)
> +                goto error;

and here...

> +        r = asprintf(&v,
> +                     "type='signal',"
> +                     "sender='org.freedesktop.login1',"
> +                     "interface='org.freedesktop.login1.Session',"
> +                     "member='PauseDevice',"
> +                     "path='%s'",
> +                     mon->spath);

asprintf() is actually disgustingly slow. Not that it would matter much
int this case here, but strjoin() is much nicer and measurably faster
for simple concatenation.

> +static int gfx_logind_prepare(sd_gfx_monitor *mon) {
> +        _cleanup_free_ char *sid = NULL;
> +        _cleanup_bus_error_free_ sd_bus_error err = SD_BUS_ERROR_NULL;
> +        int r;
> +
> +        sid = sd_bus_label_escape(mon->sid);
> +        if (!sid)
> +                return log_oom();
> +
> +        r = asprintf(&mon->spath, "/org/freedesktop/login1/session/%s", sid);
> +        if (r < 0)
> +                goto error;

strappend() as special case of strjoin() for only two strings is even
better. Or, in this case, since you never hand out the string you can
even use strappenda()...

Lennart

-- 
Lennart Poettering, Red Hat