Hi
On Tue, Nov 25, 2014 at 10:50 AM, Jan Synacek <jsyna...@redhat.com> wrote:
> David Herrmann <dh.herrm...@gmail.com> writes:
>> Hi Jan!
>
> Hello!
>
>> On Tue, Nov 25, 2014 at 9:23 AM, Jan Synacek <jsyna...@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?
Yeah. In debug mode, libxkbcommon can generate quite some traffic.
This is not about security, but about performance if we generate like
megabytes of data if a keymap is dumped. But on the other hand, if
someone requests this verbosity, maybe we should forward it.
Btw., why not concatenate all the output from xkbcommon? I think
bus_error_setfv() overwrites any previous error.
>>> + 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?
Quite simple:
Whenever a callback via sd-bus is called, you get an sd_bus_error
object. If you set an error on it, or if you return "r < 0", then
sd-bus generates an error-reply. Otherwise, it does nothing.
That means, if you call sd_bus_reply_method_return(bus, NULL); like we
do at the bottom of method_set_x11_keyboard(), then you really should
not have set any error before. Furthermore, if you call
sd_bus_reply_method_return(), you should directly return its value (to
make sure sd-bus correctly generates an error if it fails for whatever
reason).
>> 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.
Right. Now the question is whether it's ok to add new-lines to
bus-errors so we can concatenate all xkbcommon messages.
Furthermore, what's the expected policy? Should we let the call
succeed but still return an error? Should we add a "force" parameter?
Should we enforce the xkb-check?
Thanks
David
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
