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

[Lennart Poettering]

On Tue, 11.11.14 10:03, Jan Synacek (jsynacek at redhat.com) wrote:

> >> +                r = xkb_validate_keymaps(model, layout, variant, options, &msg);
> >> +                if (r < 0) {
> >> +                        log_error("Failed to validate X11 keyboard layout: %s", strerror(-r));
> >> +                        return sd_bus_error_set(error, SD_BUS_ERROR_FAILED, msg);
> >> +                }
> >> +
> >
> > Please return the error back to the client instead of logging it
> > away. Use sd_bus_error_setf() for that.
> 
> The errno part of the error is logged and the "digestible by the user"
> part is returned in the error message. I did this in exactly the same
> way as it was already there a few lines below (x11_write_data()
> call). Are you sure I should change it? If yes, both log_error()s should
> probably be changed.

Validation errors with stuff the caller passed in should be pased back
to the caller. System errors that the caller is not really responsible
for should end up in the logs. If we cannot write files into /etc
(because read-only or full or whatever) then this is not the caller's
fault, hence this should be logged. But if he passes complete rubbish
data, then it is.

> > I figure strv_split() does exactly the same thing, please use that.
> 
> That's what I had originally used. The problem is that it throws away
> the empty parts, which is what I don't want. I need it to return the
> empty strings after splitting as well. That's why I wrote my own that
> uses strsep(). Since I figured it's probably not needed anywhere else, I
> wrote it locally instead of introducing a new shared strv_<something>
> function.

Hmm, I see.

Maybe we should extend strv_split() with a bool (or flags field) that
allows a slight alteration of the allgorithm so that it does not
collapse multiple separators into one.

> >> +int xkb_get_keymaps(char ***list, enum keymap_state look_for, const char *layout_prefix)
> >> +{
> >> +        _cleanup_fclose_ FILE *f;
> >> +        char line[LINE_MAX];
> >> +        enum keymap_state state = NONE;
> >> +        int r;
> >> +
> >> +        f = fopen("/usr/share/X11/xkb/rules/base.lst", "re");
> >> +        if (!f) {
> >> +                log_error("Failed to open keyboard mapping list. %m");
> >> +                return -errno;
> >> +        }
> >
> > This should probably become a function call that returns errors but
> > doesn't log about them, leaving that to the caller.
> 
> Again, are you sure? The logged error is very "local" to what the code is
> trying to do.

Well, while this is not strictly followed the calls in share/*.c area
usually of the non-logging kind...

But this doesn't matter, it's OK if you leave it like it is right now
in your patch.

Lennart

-- 
Lennart Poettering, Red Hat