[systemd-devel] [PATCH] localed: forward xkbcommon errors
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();
+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));
--
1.9.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
2014-12-02 14:02 GMT+01:00 Jan Synacek jsyna...@redhat.com:
Hi,
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();
If you only need to concatenate you can use strjoin and since this is
only valid for this scope, strappenda is even more appropriate here.
+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));
--
1.9.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
On Tue, 02.12.14 14:02, 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();
+e = xkb_context_get_user_data(ctx);
+bus_error_setfv(e, SD_BUS_ERROR_INVALID_ARGS, fmt, args);
I thought the plan now was to log them at debug level but not return
them to the client?
Also, strappenda()!
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
Lennart Poettering lenn...@poettering.net writes:
On Tue, 02.12.14 14:02, 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();
+e = xkb_context_get_user_data(ctx);
+bus_error_setfv(e, SD_BUS_ERROR_INVALID_ARGS, fmt, args);
I thought the plan now was to log them at debug level but not return
them to the client?
Also, strappenda()!
Lennart
Please, disregard this patch submission. For some reason, I sent a
wrong version.
Sorry,
--
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
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
Lennart Poettering lenn...@poettering.net writes:
On Tue, 25.11.14 10:01, David Herrmann (dh.herrm...@gmail.com) wrote:
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.
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.
What precisely are the error messages libxkbcommon prints? Are they
really material to pass to the client? I mean, are they really about
some invalid data the client passed or just about something broken in
the way the system is set up? If it's the latter than I figure we
should return just a simple error to the client, and pass the full
logs to the logging stream...
Lennart
Thinking about it again, I don't think that it's a good idea to pass
those errors to the client...
A few examples:
# localectl set-x11-keymap QQ
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: libxkbcommon:
Couldn't find file symbols/QQ in include paths
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: libxkbcommon:
1 include paths searched:
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: libxkbcommon:
/usr/share/X11/xkb
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: libxkbcommon:
Abandoning symbols file (unnamed)
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: libxkbcommon:
Failed to compile xkb_symbols
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: libxkbcommon:
Failed to compile keymap
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: Cannot
compile XKB keymap for new x11 keyboard layout ('' / 'QQ' / '' / ''): Invalid
argument
# localectl set-x11-keymap us pc105 QQ
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]: libxkbcommon:
Couldn't process include statement for 'us(QQ)'
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]: libxkbcommon:
Abandoning symbols file (unnamed)
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]: libxkbcommon:
Failed to compile xkb_symbols
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]: libxkbcommon:
Failed to compile keymap
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]: Cannot
compile XKB keymap for new x11 keyboard layout ('pc105' / 'us' / 'QQ' / ''):
Invalid argument
Those errors are just stupid. Maybe they would make sense if there was
also libxkbcommon code in front of me... But from the user's
perspective, they say nothing useful. I added the prefix so the errors
at least have some context.
My original patch would say something like No such layout 'QQ' in the
first case and No variant 'QQ' for layout 'us' in the second.
So, is it worth logging this information? Any other ideas?
Cheers,
--
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
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
On Mon, 01.12.14 12:04, Jan Synacek (jsyna...@redhat.com) wrote:
Thinking about it again, I don't think that it's a good idea to pass
those errors to the client...
A few examples:
# localectl set-x11-keymap QQ
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]:
libxkbcommon: Couldn't find file symbols/QQ in include paths
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]:
libxkbcommon: 1 include paths searched:
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]:
libxkbcommon: /usr/share/X11/xkb
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]:
libxkbcommon: Abandoning symbols file (unnamed)
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]:
libxkbcommon: Failed to compile xkb_symbols
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]:
libxkbcommon: Failed to compile keymap
Dec 01 12:02:06 fedora-rawhide-systemd-virt systemd-localed[877]: Cannot
compile XKB keymap for new x11 keyboard layout ('' / 'QQ' / '' / ''): Invalid
argument
# localectl set-x11-keymap us pc105 QQ
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]:
libxkbcommon: Couldn't process include statement for 'us(QQ)'
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]:
libxkbcommon: Abandoning symbols file (unnamed)
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]:
libxkbcommon: Failed to compile xkb_symbols
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]:
libxkbcommon: Failed to compile keymap
Dec 01 12:03:03 fedora-rawhide-systemd-virt systemd-localed[896]: Cannot
compile XKB keymap for new x11 keyboard layout ('pc105' / 'us' / 'QQ' / ''):
Invalid argument
Those errors are just stupid. Maybe they would make sense if there was
also libxkbcommon code in front of me... But from the user's
perspective, they say nothing useful. I added the prefix so the errors
at least have some context.
My original patch would say something like No such layout 'QQ' in the
first case and No variant 'QQ' for layout 'us' in the second.
So, is it worth logging this information? Any other ideas?
Maybe log it, but downgrade it to LOG_DEBUG?
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
On Tue, 25.11.14 09:23, Jan Synacek (jsyna...@redhat.com) wrote:
#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();
Looks like a candidate for strappenda()
+e = xkb_context_get_user_data(ctx);
+bus_error_setfv(e, SD_BUS_ERROR_INVALID_ARGS, fmt, args);
}
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
On Tue, 25.11.14 10:01, David Herrmann (dh.herrm...@gmail.com) wrote: 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 think strappenda() is more appropriate. Format strings are generally *not* unbounded in their length. LINE_MAX is the best way to chicken out of doing precise allocations of things, but it's still chickening out. And since format strings ar not unbounded in their size we really should use strappenda() for it. And if not, then at least strappend()... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
On Tue, 25.11.14 10:01, David Herrmann (dh.herrm...@gmail.com) wrote: 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. 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. What precisely are the error messages libxkbcommon prints? Are they really material to pass to the client? I mean, are they really about some invalid data the client passed or just about something broken in the way the system is set up? If it's the latter than I figure we should return just a simple error to the client, and pass the full logs to the logging stream... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] localed: forward xkbcommon errors
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();
+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));
--
1.9.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
Hi Jan!
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).
+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.
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.
Thanks
David
--
1.9.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
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?
+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
signature.asc
Description: PGP signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] localed: forward xkbcommon errors
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
