[systemd-devel] [PATCH] localed: forward xkbcommon errors

[Jan Synacek]

David Herrmann <dh.herrmann at gmail.com> writes:
> Hi Jan!

Hello!

> On Tue, Nov 25, 2014 at 9:23 AM, Jan Synacek <jsynacek at redhat.com> wrote:
>> The errors are prefixed with "libxkbcommon", because they are quite
>> confusing. With the prefix, we at least know where they come from.
>> ---
>>  src/locale/localed.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/locale/localed.c b/src/locale/localed.c
>> index 4e56382..ea54798 100644
>> --- a/src/locale/localed.c
>> +++ b/src/locale/localed.c
>> @@ -1011,10 +1011,16 @@ static int method_set_vc_keyboard(sd_bus *bus, sd_bus_message *m, void *userdata
>>
>>  #ifdef HAVE_XKBCOMMON
>>  static void log_xkb(struct xkb_context *ctx, enum xkb_log_level lvl, const char *format, va_list args) {
>> -        /* suppress xkb messages for now */
>> +        _cleanup_free_ char *fmt = NULL;
>> +        sd_bus_error *e;
>> +
>> +        if (asprintf(&fmt, "libxkbcommon: %s", format) < 0)
>> +                (void) log_oom();
>
> I think you can safely use:
>
> char fmt[LINE_MAX];
>
> snprintf(fmt, sizeof(fmt), "libxkbcommon: %s", format);
> e = xkb_context_get_user_data(ctx);
> bus_error_setfv(e, SD_BUS_ERROR_INVALID_ARGS, fmt, args);
>
> We use LINE_MAX as explicit limit on a lot of these calls (and I think
> it makes sense to prevent unbound allocations).

I don't understand this. What do you mean by "unbound allocation"? That
libxkbcommon could, in theory, return a huge format that would exhaust
all memory?

>> +        e = xkb_context_get_user_data(ctx);
>> +        bus_error_setfv(e, SD_BUS_ERROR_INVALID_ARGS, fmt, args);
>>  }
>>
>> -static int verify_xkb_rmlvo(const char *model, const char *layout, const char *variant, const char *options) {
>> +static int verify_xkb_rmlvo(const char *model, const char *layout, const char *variant, const char *options, sd_bus_error *error) {
>>          const struct xkb_rule_names rmlvo = {
>>                  .model          = model,
>>                  .layout         = layout,
>> @@ -1033,6 +1039,7 @@ static int verify_xkb_rmlvo(const char *model, const char *layout, const char *v
>>                  goto exit;
>>          }
>>
>> +        xkb_context_set_user_data(ctx, (void *)error);
>>          xkb_context_set_log_fn(ctx, log_xkb);
>>
>>          km = xkb_keymap_new_from_names(ctx, &rmlvo, XKB_KEYMAP_COMPILE_NO_FLAGS);
>> @@ -1049,7 +1056,7 @@ exit:
>>          return r;
>>  }
>>  #else
>> -static int verify_xkb_rmlvo(const char *model, const char *layout, const char *variant, const char *options) {
>> +static int verify_xkb_rmlvo(const char *model, const char *layout, const char *variant, const char *options, sd_bus_error *error) {
>>          return 0;
>>  }
>>  #endif
>> @@ -1087,7 +1094,7 @@ static int method_set_x11_keyboard(sd_bus *bus, sd_bus_message *m, void *userdat
>>                      (options && !string_is_safe(options)))
>>                          return sd_bus_error_set_errnof(error, -EINVAL, "Received invalid keyboard data");
>>
>> -                r = verify_xkb_rmlvo(model, layout, variant, options);
>> +                r = verify_xkb_rmlvo(model, layout, variant, options, error);
>>                  if (r < 0)
>>                          log_warning("Cannot compile XKB keymap for new x11 keyboard layout ('%s' / '%s' / '%s' / '%s'): %s",
>>                                      strempty(model), strempty(layout), strempty(variant), strempty(options), strerror(-r));
>
> I explicitly ignore errors from verify_xkb_rmlvo() and proceed.
> libxkbcommon is still not 100% compatible to libxkb (and doesn't
> intend to be that, I guess). As we write X11 configs here, I just
> continue with a warning.
>
> If you call bus_error_setfv(), then sd-bus will return a method-error
> to the caller. However, you also send a method-return in
> method_set_x11_keyboard(). You thus end up with 2 calls.

Hmm, I thought that bus_error_setfv() only fills the error struct. Is
there any documentation that desribes how the internal bus_* stuff
works?

> I'm not sure how to solve this. Furthermore, libxkbcommon can print a
> lot of informational warnings, and we shouldn't use just the last one.
> One idea would be to copy the same logic into localectl. You could
> also specify XKB_LOG_LEVEL and XKB_LOG_VERBOSITY as environment to get
> more information there.
> Yes, this would mean compiling the keymap twice, but it's for the sake
> of debugging output so I think it's fine.

I don't think that would work. You would have the same code on the
client and on the server and that might not be the same xkb
configuration.

> Thanks
> David

-- 
Jan Synacek
Software Engineer, Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20141125/34e7f345/attachment.sig>