[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume.
From: |
Ludovic Courtès |
Subject: |
[bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume. |
Date: |
Mon, 24 Feb 2020 17:02:49 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Hi Maxim,
Resuming review of this series… Sorry for the delay!
Maxim Cournoyer <address@hidden> skribis:
>>> From 97d8a635eba34c7cf0708e99bf77ef9bad1344bf Mon Sep 17 00:00:00 2001
>>> From: Maxim Cournoyer <address@hidden>
>>> 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 <address@hidden>
>>> 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’.