wip! many: inject XKB configs in kcmdline #16338

ZeyadYasser · 2025-12-03T20:06:50Z

This draft PR allows snapd to inject XKB configs as a kernel cmdline argument to be consumed by plymouth-set-keymap.service from #16269 early in boot to set the correct keyboard layout configuration for plymouth.

This is only a draft to discuss and converge on the approach, afterwards it will be closed and split into several tested PRs.

I went with a generic approach for "extra snapd kcmdline args" that we might utilize in the future when we need to inject similar configurations for early boot in FDE systems.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

github-actions · 2025-12-03T20:18:33Z

Wed Dec 3 20:29:54 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/19907553269

No spread failures reported

BAMF0 · 2025-12-04T08:13:04Z

osutil/xkb.go

+
+	config := &XKBConfig{
+		Layouts:  strings.Split(vals["X11Layout"], ","),
+		Models:   strings.Split(vals["X11Model"], ","),


The xkb configuration file only supports specifying one model. It might be a good idea to change the datatype from []string to just string

BAMF0 · 2025-12-04T08:14:14Z

osutil/xkb.go

+)
+
+type XKBConfig struct {
+	Models   []string


The configuration file can only specify one model so we may want to rename this to Model and change the datatype to just string.

This would likely mean that the Model field needs to be parsed separately from the map used by the other fields.

Yes, I do not think we can have multiple models.

BAMF0 · 2025-12-04T08:31:41Z

osutil/xkb.go

+}
+
+func CurrentXKBConfig() (*XKBConfig, error) {
+	conn, err := dbusutil.SystemBus()


Should this function verify the configuration as well? AFAIK, the default model and layout if not specified are pc105 and us, but if Variants is not empty we expect the same number of elements in Layouts. I would say that verification is unnecessary however, if there are existing assertions that prevent an invalid state.

valentindavid · 2025-12-08T11:46:03Z

osutil/xkb.go

+	config := &XKBConfig{
+		Layouts:  strings.Split(vals["X11Layout"], ","),
+		Models:   strings.Split(vals["X11Model"], ","),
+		Variants: strings.Split(vals["X11Variant"], ","),


We should check that Variants and Layouts have the same lentgth and warn if not.

valentindavid · 2025-12-08T13:17:21Z

osutil/xkb.go

+		return nil, err
+	}
+
+	xkbConfigFile := filepath.Join(dirs.GlobalRootDir, "/etc/default/keyboard")


Does inotify_add_watch follow symbolic link? If not we should also watch /etc/vconsole.conf. I expect at some point /etc/default/keyboard becomes the symlink as a backward compliant file.

valentindavid · 2025-12-08T13:20:40Z

overlord/devicestate/devicemgr.go

+
+	simplified := fmt.Sprintf("%s,%s,%s,%s", layout, model, variant, opts)
+
+	updated, err := setExtraSnapdKernelCommandLineArg(m.state, extraSnapdKernelCmdlineArgXKB, simplified)


Should we not cache here the value? I suppose we can get lots of notifications, and not all will modify the keyboard configuration.

if the config didn't change, setExtraSnapdKernelCommandLineArg is a no-op.

pedronis · 2025-12-09T13:19:46Z

@ZeyadYasser there's a conflict here now

pedronis

some questions

pedronis · 2025-12-09T13:39:46Z

overlord/devicestate/kcmdline.go

+	taskParam string) (string, error) {
+
+	var value string
+	err := tsk.Get(taskParam, &value)


is this code used only in tests?

This helper was unchanged, only moved to a new file and renamed. I looked in the code base, This is used when setting cmdline-append config:

snapd/overlord/configstate/configcore/kernel.go

Lines 161 to 168 in 4fc4cfa

t := st.NewTask("update-gadget-cmdline",

"Update kernel command line from system configuration")

// Pass options to the task (changes in the options are not

// committed yet so the task cannot simply get them from the

// configuration)

for _, arg := range args {

t.Set(arg.name, arg.cmdline)

}

pedronis · 2025-12-09T13:45:08Z

overlord/devicestate/kcmdline.go

+	t := st.NewTask("update-managed-boot-config", summary)
+	t.Set("no-restart", true)
+
+	chg := st.NewChange("set-extra-snapd-kcmdline-arg", summary)


what happens if this kind of changes happens between a applyCmdlineChangeKind (see configcore/kernel.go) and the point that code really commits the change to the config, will we use the additions? do we have conflict logic atm moment that prevent that one and the current handlers changes interfering? but that will not cover this case and we can't use conflicts

I don't understand the first question, given that the additions are added to the state (with the state lock held and later released) shouldn't the additions always be taken into consideration when any of the kernel cmdline update tasks run regardless of the parent change? and since we create a change anyway, it shouldn't matter if some ongoing change that updates the kcmdline missed it.

For the second part regarding conflicts, you are right we can't use conflicts, instead we should block them from running in parallel using AddBlocked because during reseals the state might actually be unlocked. The blocking mechanism here might actually even be more important than fde/kcmdline blocking we were discussing before.

consider that the configcore code is using a Change so I'm quite sure it needs to release the lock, so we are not keeping the lock that's why I'm worried about this clashing with such a change

I thought deeper about this, conflicts will minimize the window where this could happen but not completely fix it, which indicates that this is a bug in snapd not specific to xkb arg injection (e.g. imagine a gadget refresh change runs right after the core config change finished but before the config transaction was committed).

One thing we can do, is extend the conflict detection CheckUpdateKernelCommandLineConflict to fail if there is some hook running (which should cover the core configs) using HookManager.NumRunningHooks, I wonder if there is a way to only fail for core config "hijacked" hook to avoid conflict detection failing for irrelevant cases.

This should fix the conflict detection logic for snapd and gadget refresh changes.

For xkb configs, it can simply wait until the conflict detection doesn't return an error because the listener runs in it own go routine, or to make things simpler it can just fail and the new values will be picked up in the next ensure loop.

If I understand correctly, this is only a problem if some other kcmdline changing change is running after core config change is triggered were the config transaction wouldn't have been committed yet but running before is fine because of the conflict detection in place, no?

I have a better idea, we could do blocking/conflict detection on the core configure-hook run-hook task. We could add some helper to hookstate to check if a given task is a core configure hook, we can even add extra logic to detect if is a core configure hook affecting specific parameters since we only care about cmdline-append and dangerous-cmdline-append. To make it generic it could be parameterized by the hookName and snapName.

This works because it the config transaction should have been committed if the parent run-hook task finishes.

This sounds like an existing bug that was only highlighted by this PR.

A much simpler approach to achieve the same outcome is to inject the parent run-hook task-id into applyCmdlineChangeKind change's state, and update the conflict detection to detect that and fail if the parent run-hook task is not ready.

ZeyadYasser requested review from bboozzoo, pedronis and valentindavid December 3, 2025 20:06

ZeyadYasser added Needs Samuele review Needs a review from Samuele before it can land FDE Marathon labels Dec 3, 2025

github-actions bot added the Run only one system Only runs spread tests on one system label Dec 3, 2025

many: inject XKB configs in kcmdline

77fbd44

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

ZeyadYasser force-pushed the extra-snapd-kcmdline-args-spike branch from 7fc6274 to 77fbd44 Compare December 3, 2025 20:18

BAMF0 reviewed Dec 4, 2025

View reviewed changes

valentindavid reviewed Dec 8, 2025

View reviewed changes

pedronis reviewed Dec 9, 2025

View reviewed changes


		simplified := fmt.Sprintf("%s,%s,%s,%s", layout, model, variant, opts)

		updated, err := setExtraSnapdKernelCommandLineArg(m.state, extraSnapdKernelCmdlineArgXKB, simplified)

	t := st.NewTask("update-gadget-cmdline",
	"Update kernel command line from system configuration")
	// Pass options to the task (changes in the options are not
	// committed yet so the task cannot simply get them from the
	// configuration)
	for _, arg := range args {
	t.Set(arg.name, arg.cmdline)
	}

wip! many: inject XKB configs in kcmdline #16338

Are you sure you want to change the base?

wip! many: inject XKB configs in kcmdline #16338

Uh oh!

Conversation

ZeyadYasser commented Dec 3, 2025

Uh oh!

github-actions bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No spread failures reported

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BAMF0 Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis commented Dec 9, 2025

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeyadYasser Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Dec 3, 2025 •

edited

Loading

BAMF0 Dec 4, 2025 •

edited

Loading

ZeyadYasser Dec 11, 2025 •

edited

Loading