Hi Maxim, Resuming review of this series… Sorry for the delay! Maxim Cournoyer skribis: >>> From 97d8a635eba34c7cf0708e99bf77ef9bad1344bf Mon Sep 17 00:00:00 2001 >>> From: Maxim Cournoyer >>> Date: Tue, 11 Feb 2020 12:57:29 -0500 >>> Subject: [PATCH 2/9] gnu: linux-boot: Ensure volatile root is mounted >>> read-only. >>> >>> * gnu/build/linux-boot.scm (mount-root-file-system): Ensure MS_RDONLY is >>> present among the root file system flags when VOLATILE-ROOT? is #t. >> >> (You can drop the “gnu:” prefix.) > > Done. > > I never know before looking at past logs (and then sometimes it's a > mixed bag). Is there any mechanical process for selecting the right > commit prefix? :-) “gnu:” is for changes to (gnu packages). The idea is that the prefix should reflect what subsystem the commit is modifying. But yeah, looking at ‘git log’ can be inspiring. :-) >>> @item --root=@var{root} >>> -Mount @var{root} as the root file system. @var{root} can be a >>> -device name like @code{/dev/sda1}, a file system label, or a file system >>> -UUID. >>> +Mount @var{root} as the root file system. @var{root} can be a device >>> +name like @code{/dev/sda1}, a file system label, or a file system UUID. >>> +When unspecified, the device name from the root file system of the >>> +operating system declaration is used. >> >> Oh! Does it always work? That makes me wonder why we’ve been carrying >> ‘--root’ and I’m not sure if I’m forgetting a good reason to do it that >> way. > > If the documentation is accurate, it should :-), given that --root gets > written as a string to the GRUB configuration file, and that the doc > says it's possible to give it as a device name, label or UUID. Yes, ‘--root’ can resolve labels and UUIDs; my question was more about why we have it in the first place. > About why providing options such as --root or --root-options in the > first place; I pondered about this as well, especially after making the > file systems from operating system able to be mounted with all their > (file system independent -- more on that later) options. A reason I > came up with was that it allows to experiment at the GRUB command line > and change the root device, or perhaps the root options. One use case > would be debugging the right options to pass to a file system driver in > case of a mistake in the operating system declaration. Yes, that makes sense. It’s certainly useful to have ‘--root’ at least as an option. >> The main issue I see with this change is that mount(2) takes raw strings >> for the options. There’s a convention to have those strings look like >> “KEY1=VALUE1,KEY2=VALUE2”, but it’s just a convention. >> >> As a rule of thumb, I’d rather have our interface be as close as >> possible to the actual mount(2) interface, which means taking strings. >> >> Now, we can surely add helper procedures to parse options that follow >> the above conventions. >> >> WDYT? > > To me, it's an implementation detail that I'd rather abstract away (or > make optional, like in this patch). Just like we provide a higher level > configuration for services instead of requiring the user to input the > configuration in the native format of the tool (or allowing for both). > The idea for this format was taken from a discussion here: > http://issues.guix.info/issue/33517#3. > > Are we really targeting mount(2)? The commit > 9d3053819dfd834a1c29a03427c41d8524b8a7d5 (which you co-authored :-)) > mentions 'man 8 mount' for the file system options. Right, mount(8) documents file system options that can be passed to mount(2). What does it mean to target mount(8) vs. mount(2)? To me, mount(8) is a CLI to mount(2) that provides additional features to make the CLI more convenient: the “defaults” option, a way to pass mount(2) flags as options (like “ro”, “remount”, “bind”), /etc/fstab handling, etc. Guix System handles /etc/fstab differently and “defaults” makes little sense in our API (one can just use leave the default value of the ‘options’ field.) I think mount(8) is actually a good illustration of what not to do. It ends up mixing things that are separate in the mount(2) API, and that doesn’t improve clarity and future-proof-ness (what if a file system has a “bind” option, etc.). But again, I think the helper procedures that you propose to move back and forth between the string and the alist representations are very welcome. I just wouldn’t hard-code that directly in our API. WDYT? >>> From 67135c925b07f2e077b4cd852e07178691a10164 Mon Sep 17 00:00:00 2001 >>> From: Maxim Cournoyer >>> Date: Tue, 11 Feb 2020 14:14:36 -0500 >>> Subject: [PATCH 6/9] gnu: linux-boot: Honor the "--root-options" kernel >>> argument. >>> >>> * gnu/build/linux-boot.scm (boot-system): Parse the "--root-options" kernel >>> argument, and use it when calling `mount-root-file-system'. Update doc. >>> * doc/guix.texi (Initial RAM Disk): Document the use of the "--root-options" >>> argument. >> >> Hmm do we really need this extra option? :-) > > It is not strictly needed but allows the user to experiment/troubleshoot > with the init RAM disk from GRUB as discussed earlier for --root. Do > you think it has enough value to be kept? I’d rather avoid it for now. Less code is better. :-) >> (Also, in hindsight, I think it was a mistake to call them >> ‘--something’. Following the common naming convention, we should rather >> call these options ‘gnu.something’.) > > Is this convention detailed somewhere? I haven't found it in 'Standards'. It’s a convention of the Linux kernel, I don’t know if it’s documented. That’s it! Ludo’.