[systemd-devel] [PATCH v2] localed: validate set-x11-keymap input

[David Herrmann]

Hi

On Wed, Nov 19, 2014 at 9:41 AM, Jan Synacek <jsynacek at redhat.com> wrote:
> David Herrmann <dh.herrmann at gmail.com> writes:
>> Hi
>>
>> On Fri, Nov 14, 2014 at 12:42 PM, Jan Synacek <jsynacek at redhat.com> wrote:
>>> Try to validate the input similarly to how setxkbmap does it. Multiple
>>> layouts and variants can be specified, separated by a comma. Variants
>>> can also be left out, meaning that the user doesn't want any particular
>>> variant for the respective layout.
>>>
>>> Variants are validated respectively to their layouts. First variant
>>> validates against first layout, second variant to second layout, etc. If
>>> there are more entries of either layouts or variants, only their
>>> respective counterparts are validated, the rest is ignored.
>>>
>>> Examples:
>>> $ set-x11-keymap us,cz  pc105 ,qwerty
>>> "us" is not validated, because no respective variant was specified. "cz"
>>> is checked for an existing "qwerty" variant, the check succeeds.
>>>
>>> $ set-x11-keymap us pc105 ,qwerty
>>> "us" is not validated as in the above example. The rest of the variants
>>> is ignored, because there are no respective layouts.
>>>
>>> $ set-x11-keymap us,cz pc105 qwerty
>>> "us" is validated against the "qwerty" variant, check fails, because
>>> there is no "qwerty" variant for the "us" layout.
>>>
>>> $ set-x11-keymap us,cz pc105 euro,qwerty
>>> Validation succeeds, there is the "euro" variant for the "us" layout,
>>> and "qwerty" variant for the "cz" layout.
>>>
>>> http://lists.freedesktop.org/archives/systemd-devel/2014-October/024411.html
>>
>> I didn't follow the discussion on v1, but why don't we use
>> libxkbcommon to compile the keymap? If it doesn't compile, print an
>> error (or warning and maybe optionally still proceed?).
>>
>> Sure, this would add a dependency to libxkbcommon for localed, but we
>> could make it optional. And libxkbcommon has no dependencies by
>> itself. Furthermore, set-x11-keymap is pretty useless without
>> libxkbcommon installed, anyway.
>>
>> It'd be a ~10 line patch to use
>> xkb_context_new()+xkb_keymap_from_rmlvo(). And it would be guaranteed
>> to have the same semantics as the real keymap compilation.
>>
>> Thanks
>> David
>
> For some reason, I was under the impression that depending on
> libxkbcommon would mean depending on plenty of X libraries... Using the
> library and making the dependency on it optional sounds like the best
> solution to me.
>
> Waiting for more opinions.

I now pushed something to -git, see:

    commit d4f5a1f47dbd04f26f2ddf951c97c4cb0ebbbe62
    Author: David Herrmann <dh.herrmann at gmail.com>
    Date:   Mon Nov 24 15:12:42 2014 +0100

        localed: validate xkb keymaps

I think duplicating the xkb-parsers is something we should avoid. Ran
Benita was even working on a v2 format to drop all the legacy bits
no-one uses in the old, crufty xkb format. I dislike having a fixed
parser in systemd. Sure, our --list-layouts option needs to do this,
but maybe we can drop that some day, too.

I pushed a fix to test-compile keymaps on set-x11-keymap calls. So
far, I only print a warning if it fails. Please feel free to post
patches to extend this. For instance, I think we might want to do that
in localectl (maybe even forward the xkb-logs then) and fail hard if
the compilation fails (even though I dislike --force options..).

I hope my commit is something to base further work on. I tried keeping
it as non-intrusive as possible.

Thanks
David