On Tue, 04.11.14 12:05, Jan Synacek (jsyna...@redhat.com) wrote: > diff --git a/src/locale/localectl.c b/src/locale/localectl.c > index d4a2d29..8f9e4da 100644 > --- a/src/locale/localectl.c > +++ b/src/locale/localectl.c > @@ -46,6 +46,7 @@ > #include "virt.h" > #include "fileio.h" > #include "locale-util.h" > +#include "xkb-util.h" > > static bool arg_no_pager = false; > static bool arg_ask_password = true; > @@ -389,14 +390,7 @@ static int set_x11_keymap(sd_bus *bus, char **args, > unsigned n) { > static int list_x11_keymaps(sd_bus *bus, char **args, unsigned n) { > _cleanup_fclose_ FILE *f = NULL; > _cleanup_strv_free_ char **list = NULL; > - char line[LINE_MAX]; > - enum { > - NONE, > - MODELS, > - LAYOUTS, > - VARIANTS, > - OPTIONS > - } state = NONE, look_for; > + enum keymap_state look_for;
While we don#t follow this rule too strictly we usually define typdefs for enums and name them in CamelCase. Hence, please rename this type "KeymapState" instead of "enum keymap_state". That said, is "state" really the right name here? Shouldn't it be "field" or "component" or "item" or so? > !streq_ptr(variant, c->x11_variant) || > !streq_ptr(options, c->x11_options)) { > + _cleanup_free_ char *msg = NULL; > > if ((layout && !string_is_safe(layout)) || > (model && !string_is_safe(model)) || > @@ -1050,6 +1052,12 @@ static int method_set_x11_keyboard(sd_bus *bus, > sd_bus_message *m, void *userdat > free_and_strdup(&c->x11_options, options) < 0) > return -ENOMEM; > > + 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. Use SD_BUS_ERROR_INVALID_ARGS as error id. > + > +static char **xkb_parse_argument(const char *arg) > +{ > + _cleanup_free_ char *input; > + char *token; > + char **result = NULL; > + int r; > + > + assert(arg); > + > + input = strdup(arg); > + if (!input) > + return NULL; > + > + token = strsep(&input, ","); > + while(token) { > + r = strv_extend(&result, token); > + if (r < 0) > + return NULL; > + token = strsep(&input, ","); > + } > + > + return result; > +} Please place the opening { of a function body on the same line as the function declaration. I figure strv_split() does exactly the same thing, please use that. > + > +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. > +int xkb_validate_keymaps(const char *model, > + const char *layouts_arg, > + const char *variants_arg, > + const char *options_arg, > + char **error) > +{ > + int r; > + > + if (layouts_arg) { > + _cleanup_strv_free_ char **layouts_list = NULL; > + char **it, **layouts; > + int i = 0; > + > + r = xkb_get_keymaps(&layouts_list, LAYOUTS, NULL); > + if (r < 0) > + return r; > + > + layouts = strv_split(layouts_arg, ","); > + if (!layouts) > + return log_oom(); > + > + STRV_FOREACH(it, layouts) { > + _cleanup_strv_free_ char **variants_list = NULL; > + bool variants_left = true; > + int n; > + > + if (!strv_find(layouts_list, *it)) { > + r = asprintf(error, "Requested layout '%s' > not available.\n", *it); > + if (r < 0) > + return log_oom(); > + return -EINVAL; > + } > + > + if (variants_left && variants_arg) { > + _cleanup_strv_free_ char **variants; > + > + r = xkb_get_keymaps(&variants_list, > VARIANTS, *it); > + if (r < 0) > + return r; Hmm, reading the file over and over and over again sounds less than ideal. Maybe we should intrdouce a struct here and then make xkb_get_keymaps() return an array of structs really? Lennart -- Lennart Poettering, Red Hat _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel