[bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume.

[Maxim Cournoyer]

Hello Ludovic!

Resuming review; I hope I'll be able to bring this series to completion
this time.

[...]

> “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.  :-)

OK.  Makes sense.  Thanks for taking the time to explain!

[...]

>> 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?

I don't feel too strongly about it, so I will adapt to your preference.

[...]

>> 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.).

I think I agree.  It may be cleaner to use the API of mount(2), while
mapping the low level stuff (binary flags?) to saner symbols, like it's
already done.

> 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.

Okay.  I'll keep them around in (gnu system file-systems); they're handy
when we need to check the presence of an option in the option string.

> 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.  :-)

Done!

>>> (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.

Well, with Hurd support coming, perhaps the standards used for a
particular kernel are not too relevant? :-)

One last thing that I'll try to understand before considering this
finished: I think adding a special properties field to the file system
record might not be necessary, as it seems that the subvol option can
take an absolute path as well as a relative path (I thought it only
accepted subvolume *names*.)

Will send the 3rd revision soon.

Thank you!

Maxim