Restructure keymap files

[matt335672]

Based on #3022

Most of our users are using Linux, and hence 'evdev' key mappings. These key mappings are more comprehensive than the 'base' key mappings, in that several media keys are supported. This is why #3022 was raised.

During the course of #3022. a number of issues with the existing keymaps were identified:-

1. The code to map the RDP scancodes received from the client to X11 keycodes is in three places, namely xrdp/lang.c, genkeymap/*.c and xorgxrdp itself. This makes a wholesale change to evdev challenging and tricky to test.
2. The keymaps aren't all auto-generated, and some are out-of-date (see this comment)
3. XKB changes over time, and the old way of supporting NumLock by pretending it's a shift isn't supported in newer releases (see this comment)

This PR attempts to address these deficiencies as follows:-

1. There's now a single module to map RDP scancodes to evdev (or base) key codes in common/scancode.c.
2. The keymap file format has been updated to be indexed by RDP scancode rather than base key code. This should make it easier to triage user problems, as scancodes are readily viewable on (e.g.) https://kbdlayout.info/, and also removes the need to know the X11 keycode for the login screen or the VNC back-end.
3. Keymap files now contain a short [numlock] section
4. The X11 keycode to be used by xorgxrdp is now sent directly to xorgxrdp as part of a KEY_UP / KEY_DOWN message in place of the Unicode field. This should allow for some simplification on the keyboard driver in xorgxrdp.

This is where I think we should be going after merging #3022, but I'd appreciate some constructive criticism around this, particularly from @sasha0552 and @jsorg71

[sasha0552]

Overall, it looks good. Good job. Is it ready for testing (if I modify xorgxrdp to work with it)?

[sasha0552]

Perhaps consider defining the version as a macro instead of hardcoding it for better maintainability.

[matt335672]

Thanks - will do.

[sasha0552]

Suggested change

	sending keyboard input to a VNC server.
	sending keyboard input to a VNC server or xorgxrdp.

Or perhaps a more general backend.

[matt335672]

The keymap files aren't used for xorgxrdp - only for the login screen and VNC.

For xorgxrdp, we try to ascertain the right arguments for XKB to set the keyboard up directly and pass those through. There's a different file involved (xrdp_keyboard.ini). It's all done here:-

xrdp/libxrdp/xrdp_sec.c

Lines 359 to 597 in cc7d5ef

	/*****************************************************************************/
	static void
	xrdp_load_keyboard_layout(struct xrdp_client_info *client_info)
	{
	int fd;
	int index = 0;
	int bytes;
	struct list *names = (struct list *)NULL;
	struct list *items = (struct list *)NULL;
	struct list *values = (struct list *)NULL;
	char *item = (char *)NULL;
	char *value = (char *)NULL;
	char *q = (char *)NULL;
	char keyboard_cfg_file[256] = { 0 };
	char rdp_layout[256] = { 0 };
	const struct xrdp_keyboard_overrides *ko =
	&client_info->xrdp_keyboard_overrides;
	LOG(LOG_LEVEL_INFO, "xrdp_load_keyboard_layout: Keyboard information sent"
	" by the RDP client, keyboard_type:[0x%02X], keyboard_subtype:[0x%02X],"
	" keylayout:[0x%08X]",
	client_info->keyboard_type, client_info->keyboard_subtype,
	client_info->keylayout);
	if (ko->type != -1)
	{
	LOG(LOG_LEVEL_INFO, "overrode keyboard_type 0x%02X"
	" with 0x%02X", client_info->keyboard_type, ko->type);
	client_info->keyboard_type = ko->type;
	}
	if (ko->subtype != -1)
	{
	LOG(LOG_LEVEL_INFO, "overrode keyboard_subtype 0x%02X"
	" with 0x%02X", client_info->keyboard_subtype,
	ko->subtype);
	client_info->keyboard_subtype = ko->subtype;
	}
	if (ko->layout != -1)
	{
	LOG(LOG_LEVEL_INFO, "overrode keylayout 0x%08X"
	" with 0x%08X", client_info->keylayout, ko->layout);
	client_info->keylayout = ko->layout;
	}
	/* infer model/variant */
	/* TODO specify different X11 keyboard models/variants */
	g_memset(client_info->model, 0, sizeof(client_info->model));
	g_memset(client_info->variant, 0, sizeof(client_info->variant));
	g_strncpy(client_info->layout, "us", sizeof(client_info->layout) - 1);
	if (client_info->keyboard_subtype == 3)
	{
	/* macintosh keyboard */
	bytes = sizeof(client_info->variant);
	g_strncpy(client_info->variant, "mac", bytes - 1);
	}
	else if (client_info->keyboard_subtype == 0)
	{
	/* default - standard subtype */
	client_info->keyboard_subtype = 1;
	}
	g_snprintf(keyboard_cfg_file, 255, "%s/xrdp_keyboard.ini", XRDP_CFG_PATH);
	LOG(LOG_LEVEL_DEBUG, "keyboard_cfg_file %s", keyboard_cfg_file);
	fd = g_file_open_ro(keyboard_cfg_file);
	if (fd >= 0)
	{
	int section_found = -1;
	char section_rdp_layouts[256] = { 0 };
	char section_layouts_map[256] = { 0 };
	names = list_create();
	names->auto_free = 1;
	items = list_create();
	items->auto_free = 1;
	values = list_create();
	values->auto_free = 1;
	file_read_sections(fd, names);
	for (index = 0; index < names->count; index++)
	{
	q = (char *)list_get_item(names, index);
	if (g_strncasecmp("default", q, 8) != 0)
	{
	int i;
	file_read_section(fd, q, items, values);
	for (i = 0; i < items->count; i++)
	{
	item = (char *)list_get_item(items, i);
	value = (char *)list_get_item(values, i);
	LOG(LOG_LEVEL_DEBUG, "xrdp_load_keyboard_layout: item %s value %s",
	item, value);
	if (g_strcasecmp(item, "keyboard_type") == 0)
	{
	int v = g_atoi(value);
	if (v == client_info->keyboard_type)
	{
	section_found = index;
	}
	}
	else if (g_strcasecmp(item, "keyboard_subtype") == 0)
	{
	int v = g_atoi(value);
	if (v != client_info->keyboard_subtype &&
	section_found == index)
	{
	section_found = -1;
	break;
	}
	}
	else if (g_strcasecmp(item, "rdp_layouts") == 0)
	{
	if (section_found != -1 && section_found == index)
	{
	g_strncpy(section_rdp_layouts, value, 255);
	}
	}
	else if (g_strcasecmp(item, "layouts_map") == 0)
	{
	if (section_found != -1 && section_found == index)
	{
	g_strncpy(section_layouts_map, value, 255);
	}
	}
	else if (g_strcasecmp(item, "model") == 0)
	{
	if (section_found != -1 && section_found == index)
	{
	bytes = sizeof(client_info->model);
	g_memset(client_info->model, 0, bytes);
	g_strncpy(client_info->model, value, bytes - 1);
	}
	}
	else if (g_strcasecmp(item, "variant") == 0)
	{
	if (section_found != -1 && section_found == index)
	{
	bytes = sizeof(client_info->variant);
	g_memset(client_info->variant, 0, bytes);
	g_strncpy(client_info->variant, value, bytes - 1);
	}
	}
	else if (g_strcasecmp(item, "options") == 0)
	{
	if (section_found != -1 && section_found == index)
	{
	bytes = sizeof(client_info->options);
	g_memset(client_info->options, 0, bytes);
	g_strncpy(client_info->options, value, bytes - 1);
	}
	}
	else
	{
	/*
	* mixing items from different sections will result in
	* skipping over current section.
	*/
	LOG(LOG_LEVEL_DEBUG, "xrdp_load_keyboard_layout: skipping "
	"configuration item - %s, continuing to next "
	"section", item);
	break;
	}
	}
	list_clear(items);
	list_clear(values);
	}
	}
	if (section_found == -1)
	{
	g_memset(section_rdp_layouts, 0, sizeof(char) * 256);
	g_memset(section_layouts_map, 0, sizeof(char) * 256);
	// read default section
	file_read_section(fd, "default", items, values);
	for (index = 0; index < items->count; index++)
	{
	item = (char *)list_get_item(items, index);
	value = (char *)list_get_item(values, index);
	if (g_strcasecmp(item, "rdp_layouts") == 0)
	{
	g_strncpy(section_rdp_layouts, value, 255);
	}
	else if (g_strcasecmp(item, "layouts_map") == 0)
	{
	g_strncpy(section_layouts_map, value, 255);
	}
	}
	list_clear(items);
	list_clear(values);
	}
	/* load the map */
	file_read_section(fd, section_rdp_layouts, items, values);
	for (index = 0; index < items->count; index++)
	{
	int rdp_layout_id;
	item = (char *)list_get_item(items, index);
	value = (char *)list_get_item(values, index);
	rdp_layout_id = g_htoi(value);
	if (rdp_layout_id == client_info->keylayout)
	{
	g_strncpy(rdp_layout, item, 255);
	break;
	}
	}
	list_clear(items);
	list_clear(values);
	file_read_section(fd, section_layouts_map, items, values);
	for (index = 0; index < items->count; index++)
	{
	item = (char *)list_get_item(items, index);
	value = (char *)list_get_item(values, index);
	if (g_strcasecmp(item, rdp_layout) == 0)
	{
	bytes = sizeof(client_info->layout);
	g_strncpy(client_info->layout, value, bytes - 1);
	break;
	}
	}
	list_delete(names);
	list_delete(items);
	list_delete(values);
	LOG(LOG_LEVEL_INFO, "xrdp_load_keyboard_layout: model [%s] variant [%s] "
	"layout [%s] options [%s]", client_info->model,
	client_info->variant, client_info->layout, client_info->options);
	g_file_close(fd);
	}
	else
	{
	LOG(LOG_LEVEL_ERROR, "xrdp_load_keyboard_layout: error opening %s",
	keyboard_cfg_file);
	}
	}

[matt335672]

I think it's ready for updating the xorgxrdp interface. The only possible change in this area is to pass the rules over from xrdp in the same way as we're passing the model and variant.

Next I'm going to look at getting manpages done for xrdp-genkeymap and the keymap files themselves, so I won't be messing much with the functionality.

[matt335672]

I've updated a few things to make this more platform independent. The changes are all around the genkeymap utility.

The scancode module now understands both base/xfree86 and evdev keycode mappings - it's no longer a compile-time option. This is used by the genkeymap utility to select the correct table at runtime. It's similar to what it was doing before, but the tables are separate from each other within the scancode module. This should aid maintainability and make it possible to add other tables if we ever need to.

I've verified manually that the only differences between keymap files generated using 'evdev' rules and 'base' rules are the scancode mappings for extra keys. This is what we'd expect with the keymap tables now being based on RDP scancodes.

There's now a separate manpage documenting the key mapping file format, and the manpage for xrdp-genkeymap has been updated.

Comment still very welcome. If I get nothing by around the middle of next week I'll merge this, and we can get on with updating and (hopefully) simplifying xorgxrdp.

[matt335672]

One last change; I'm quite keen on checking that we can reproduce keymap files, having gotten into a state where these are not maintainable. I've added code to the xrdp-genkeymap command to query the X server for the setxkbmap settings in effect when the keymap file was built, and I've added these to the top of the file as a comment.

For example, the top of instfiles/km-00000411.ini now looks like this:-

# Created by xrdp-genkeymap V0.10.80
# Key code set: evdev+aliases(qwerty)
# setxkbmap -rules evdev -model pc105 -layout jp -variant OADG109A
# Description: ja-JP
# Operating system: Ubuntu 22.04.4 LTS

[General]
version=2

[noshift]
01="65307:U+001B"  # Escape
. . .

This querying is now done within the xrdp-genkeymap command itself, rather than being part of the dump-keymaps.sh script. This should allow us to check that any keymaps submitted by users who maybe haven't used the dump-keymaps.sh script are reproducible.

This change introduces a new dependency on the libxkbfile-dev (or equivalent) package so we gain access to the same APIs as setxkbmap uses.

[metalefty]

@matt335672

I'm late to join this discussion, what about switching to TOML as I suggested #2659 before? I postponed merging #2659 because it breaks compatibility. This is also a breaking change so I think it is a good time to switch from our own ini parser to TOML library.

[matt335672]

Actually, I'd used the TOML parser in this PR anyway as the format was changing!

I'd forgotten #2659 was in this area. I'll have a look and figure out the best way to get both PRs merged.

[metalefty]

Ah, I only have a quick look at this so I didn't notice TOML is used since keymap files have .ini suffix. That sounds good then.

I prefer .toml suffix to indicate that keymap file is parsed by TOML v1.0.0 compliant parser.

[matt335672]

Since I'm breaking the format anyway, it's probably a good idea to rename these as .toml. That way any customisations made by users to old files won't be wiped out. I'll do that.

[matt335672]

Having reviewed #2659, I think the following should be added to this PR from it:-

1. Rename keyboard files to .toml from .ini.
2. Add a test case for loading a keyboard file. The original purpose of the test in #2659 was to check the ini and TOML parsers returned the same result. The new test case can't fall back on an ini file to test, but at the very least it gives us a way to check memory is being allocated and de-allocated properly which IIRC is part of the test added for #3065.

[matt335672]

@metalefty - I think I've addressed your latest comments. The files are all *.toml now but I need to check manpages and a couple of other things before I'm happy with it, and can start merging.

The unit test I pulled in from #2659 needed a bit of re-writing to be useful but then it found an uninitialised memory error in the keyboard map loading logic. So I'm grateful to you for pointing it out to me.

I'm planning on merging #3022 as-is so @sasha0552 gets credit, and then I'll rebase this PR on latest devel. I can fix the CI problem then.

[metalefty]

LGTM

[matt335672]

Thanks.

I've updated NEWS with suitable notices as this is a backwards-incompatible change. I'll pop something on gitter too.