Lennart Poettering <[email protected]> writes: > On Tue, 04.11.14 12:05, Jan Synacek ([email protected]) 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 [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
