On Thu, 31.01.13 21:38, Matthias Berndt (matthias_ber...@gmx.de) wrote:
> Hi,
heya,
> The attached version of the patch contains updates to the man pages.
I couldn't come up with any reason not to merge a patch like this, so
this should probably go in.
However, I'd like to ask for one change before we merge this:
The kbd leds are a per-VC setting, unlike the kbd keymap itself, but like
the font settings. For the font we currently set the font on the fg VC
and then copy it over to all other VCs via the KD_FONT_OP_COPY ioctl
which ensures the kernel only keeps a single copy of the font in memory.
To ensure behaviour of the LED stuff is close to the behaviour of the
other settings it's probably a good idea to set the LEDs not only on the
fg VT, but on all allocated ones, in a loop.
It should suffice to invoke your set_modifiers() function in some
wrapper function set_modifiers_on_all_vcs() which internally just runs
VT_GETSTATE and iterates over all allocated VCs. See
font_copy_to_all_vcs() for inspiration.
I wonder what happens if X11 is currently active on the VC that the leds
setting is applied to. Ideally, the ioctl would just fail, or at least
not have any negative impact on X11 or not break it. Have you played
around with that, by any chance?
> +
> + for (int i = 0; i < num_modifiers; ++i) {
> + if (modifier_status[i] == NULL)
> + continue;
> + switch (parse_boolean(modifier_status[i])) {
> + case 0:
> + mask &= ~modifier[i];
> + break;
> + case 1:
> + mask |= modifier[i];
> + break;
> + default:
> + log_warning("invalid value for keyboard modifier:
> \"%s\"", modifier_status[i]);
> + }
> + }
> +
> + if (ioctl(fd, KDSKBLED, mask)) {
> + r = -errno;
> + log_warning("failed to set keyboard modifier status: %s",
> strerror(errno));
> + return r;
> + }
A minor optimization might be to suppress the KDSKBLED if no change has
been made. This might be useful to avoid SELinux AVCs or so in case
people have very strict policies and nothing set.
Otherwise looks fine!
Thanks,
Lennart
--
Lennart Poettering - Red Hat, Inc.
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
