[systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

[Ran Benita]

On Mon, Oct 20, 2014 at 11:15:14AM +0200, Jan Synacek wrote:
> Ran Benita <ran234 at gmail.com> writes:
> > On Fri, Oct 17, 2014 at 02:02:13PM +0200, Jan Synacek wrote:
> >> When setting any of those using set-x11-keymap, check that their values
> >> are available on the system.
> >
> > I have only skimmed this patch, but generally:
> >
> > 1. There can only be one model.
> > 2. There can be multiple layouts and variants, comma separated. E.g.,
> >    layout=us,il variant=,lyx. The amount of layouts and variants should
> >    match (or the variants can be empty), but most stuff you throw at it
> >    will be accepted though.
> > 3. There can be multiple comma-separated options.
> >
> > Do you handle input like "layout=us,il variant=,lyx" correctly?
> >
> > Ran
> 
> Nope, I didn't realize that. I'll send a better version of the patch.
> 
> The parsing won't be perfect, though. With setxkbmap, I can do the
> following without any error:
> 
> # setxkbmap us,cz lyx ctrl:nocaps -model idontexist
> # echo $?
> 0
> 
> Note that there is no lyx variant for either us or cz. Also, the number
> of layouts and variants *doesn't* match and the model doesn't exist as
> well. (That's probably what you meant by the last sentence in 2., I'm
> just restating it to be clear).

Right, these settings are pretty permissive.

But you reminded me - it is probably not a good idea to entirely abort
the operation if the provided arguments don't validate. That's because
the XKB rules file (/usr/share/X11/xkb/rules/) support wildcards, as you
noticed with the idontexist model. These are then passed along, and some
of them are in fact valid as far as I remember, just not mentioned in the
base.lst file.

So I think this should be a warning, not an error. If the user knows
what he intended, he can ignore the error. Otherwise he will correct the
error. Not ideal, but better than preventing legal values.