Hi
On Wed, Nov 27, 2013 at 11:34 PM, Lennart Poettering
<lenn...@poettering.net> wrote:
> On Wed, 27.11.13 19:48, David Herrmann (dh.herrm...@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?
Yepp. As said in my comment on 0/12, I squashed it all in a single
header for now. If we export it, this will get moved into gfx-util.h
or similar.
>> + 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?
Sweet! Fixed.
>> +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.
Yepp, fixed.
>> +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()...
Fixed. But strappenda() doesn't work. I save that string in mon->spath
so I can reuse it for all the runtime dbus requests.
Thanks
David
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
