libxkbcommon icon indicating copy to clipboard operation
libxkbcommon copied to clipboard

Add errno like mechanism for querying errors

Open Caellian opened this issue 1 month ago • 2 comments

Most errors & warnings are reported using xkb_context_set_log_fn. This works well only for tools in this repo and doesn't provide any actionable information to applications that aren't CLIs.

Many applications that rely on libxkbcommon will not trickle down these errors to the user because it's likely their fault libxkbcommon function failed instead of user error. But none of the failures are recoverable (in practice), because code has to rely on parsing the errors from xkb_context_set_log_fn to know "I used a flag that's not supported by system libxkbcommon", or "the keymap source is malformed".

Returning null works well only if there is a single failure condition.

Further xkb_context_set_log_fn isn't usable directly from Rust FFI anyway because of vararg. It will be in future when c_variadic lands, but until then, I'd have to use a trampoline function in C, to get Rust side to handle xkb_context_set_log_fn + global static mutex + text pattern matching to return Result<Context, ContextError>.

Suggestion

Make all functions that can fail set some errno like static so that code can check the failure condition using xkb_get_error without having to parse human-readable strings. I'm not suggesting errno because only 3/all error values apply and you'd have more freedom if you defined your own enum in xkbcommon-error.h.

Error logging macro could take this additional error code and set the global on its own.

I'm assuming only a single error condition can occur, though it could be a null padded array and there could be an additional one for warnings - depending on your preferences.

Caellian avatar Nov 20 '25 17:11 Caellian

For context, here's an example of a trampoline necessary for xkb_context_set_log_fn in Rust:

#include <stdio.h>
#include <stdlib.h>
#include <xkbcommon/xkbcommon.h>

typedef void (*trampoline_t)(struct xkb_context *context, enum xkb_log_level level, const char *message);
static trampoline_t trampoline = NULL;

void handler(struct xkb_context *context, enum xkb_log_level level, const char *format, va_list args) {
    size_t len = sprintf(NULL, format, args);
    char *output = malloc(len);
    sprintf(output, format, args);
    trampoline(context, level, output);
    free(output);
}

void context_set_log_fn_trampoline(struct xkb_context *context,
    void (*callback)(struct xkb_context *context, enum xkb_log_level level, const char *message)) {
    trampoline = callback;
    xkb_context_set_log_fn(context, handler);
}

Standard handler is simply not constructable and I believe it's UB to pass a function without va_list.

Caellian avatar Nov 20 '25 19:11 Caellian

NOTE: the error message were never meant to be parsable beyond the message identifier XKB-nnn at the start.

A better error handling would be great, but I would rather avoid an errno-like mechanism and prefer passing an error ID directly to the function by reference. The issue is that it means duplicating all relevant functions, which is also not a great idea now. The new functions should however favor returning at least an error ID when relevant.

When #942 get merged, I am not sure such an error mechanism will be so important anymore: while testing available features via flags enables to avoid/recover from errors, the other errors are most probably not recoverable anyway (allocation error, invalid keymap, missing include directories, etc.).

wismill avatar Dec 02 '25 11:12 wismill