Lennart Poettering <lenn...@poettering.net> writes: > 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.
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. > 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. Oops, old habit, sorry, will fix. > 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. >> +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. >> +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? That sounds ok, I'll see what I can do. I wanted to preserve as much of the original code as I could, but maybe it wasn't the right decision. > Lennart Thanks for the review, -- Jan Synacek Software Engineer, Red Hat
signature.asc
Description: PGP signature
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel