unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: "Ludovic Courtès" <ludo@gnu.org>
To: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Cc: 37305@debbugs.gnu.org
Subject: [bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume.
Date: Mon, 24 Feb 2020 17:02:49 +0100	[thread overview]
Message-ID: <87imjwkm6u.fsf@gnu.org> (raw)
In-Reply-To: <87blpzozz7.fsf@gmail.com> (Maxim Cournoyer's message of "Sun, 16 Feb 2020 00:36:28 -0500")

Hi Maxim,

Resuming review of this series…  Sorry for the delay!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>>> From 97d8a635eba34c7cf0708e99bf77ef9bad1344bf Mon Sep 17 00:00:00 2001
>>> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>>> 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 <maxim.cournoyer@gmail.com>
>>> 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’.

  parent reply	other threads:[~2020-02-24 16:04 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05  0:20 [bug#37305] [PATCH] Allow booting from a Btrfs subvolume Maxim Cournoyer
2019-09-08 16:10 ` Christopher Baines
2019-09-22 21:43 ` Ludovic Courtès
2020-02-12  8:47   ` Maxim Cournoyer
2020-02-13 20:27     ` [bug#37305] [PATCH V2] " Maxim Cournoyer
2020-02-14 17:22       ` Ludovic Courtès
2020-02-16  5:36         ` Maxim Cournoyer
2020-02-16 11:11           ` [bug#37305] Making system installation tests faster Ludovic Courtès
2020-02-18 13:37             ` Maxim Cournoyer
2020-02-18 21:27               ` Maxim Cournoyer
2020-03-07  4:01                 ` Maxim Cournoyer
2020-02-24 16:02           ` Ludovic Courtès [this message]
2020-03-03  5:00             ` [bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume Maxim Cournoyer
2020-02-24 14:23         ` [bug#37305] [PATCH V3] " Maxim Cournoyer
2020-02-19  2:52 ` [bug#37305] Allow booting from a Btrfs subvolume [review part 2] Maxim Cournoyer
2020-02-20  9:55   ` Ludovic Courtès
2020-03-18 15:27     ` maxim.cournoyer
2020-05-17 13:29       ` Pierre Neidhardt
2020-05-17 16:13         ` [bug#37305] [PATCH v3] Allow booting from a Btrfs subvolume Maxim Cournoyer
2020-05-17 16:37           ` Pierre Neidhardt
2020-05-17 19:05             ` Pierre Neidhardt
2020-05-17 19:09               ` Pierre Neidhardt
2020-05-17 19:48                 ` Pierre Neidhardt
2020-05-18  1:16                   ` Maxim Cournoyer
2020-05-18  8:54                     ` Pierre Neidhardt
2020-05-17 20:22                 ` Pierre Neidhardt
2020-05-18  0:49                   ` Maxim Cournoyer
2020-05-18 21:55           ` Ludovic Courtès
2020-05-20 12:44             ` Maxim Cournoyer
2020-05-20 12:44             ` bug#37305: " Maxim Cournoyer
2020-05-20 13:29               ` [bug#37305] " Pierre Neidhardt
2020-05-20 22:03               ` Ludovic Courtès
2020-05-21  6:58                 ` Pierre Neidhardt
2020-05-28  4:30                   ` Maxim Cournoyer
2020-05-28  8:26                     ` Pierre Neidhardt
2020-05-29 21:14                       ` Maxim Cournoyer
2020-05-28 12:30                     ` Ludovic Courtès
2020-05-30  2:00                       ` Maxim Cournoyer
2020-05-30  7:32                         ` Pierre Neidhardt
2020-05-30  7:32                         ` Pierre Neidhardt
2020-05-31  2:44                           ` Maxim Cournoyer
2020-05-31  7:32                             ` Pierre Neidhardt
2020-05-17 14:03       ` [bug#37305] Allow booting from a Btrfs subvolume [review part 2] Pierre Neidhardt
2020-05-17 16:16         ` Maxim Cournoyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87imjwkm6u.fsf@gnu.org \
    --to=ludo@gnu.org \
    --cc=37305@debbugs.gnu.org \
    --cc=maxim.cournoyer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).