From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:43206) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j8zg0-0003Jw-0h for guix-patches@gnu.org; Tue, 03 Mar 2020 00:01:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j8zfy-000075-LK for guix-patches@gnu.org; Tue, 03 Mar 2020 00:01:03 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:60860) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1j8zfy-000071-Ho for guix-patches@gnu.org; Tue, 03 Mar 2020 00:01:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1j8zfy-0007Ek-Fr for guix-patches@gnu.org; Tue, 03 Mar 2020 00:01:02 -0500 Subject: [bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume. Resent-Message-ID: From: Maxim Cournoyer References: <87sgpby4p9.fsf@gmail.com> <87y2yg3t3s.fsf@gnu.org> <87k14sfaz7.fsf@gmail.com> <87lfp6b5cs.fsf_-_@gmail.com> <8736bdf5il.fsf@gnu.org> <87blpzozz7.fsf@gmail.com> <87imjwkm6u.fsf@gnu.org> Date: Tue, 03 Mar 2020 00:00:05 -0500 In-Reply-To: <87imjwkm6u.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Mon, 24 Feb 2020 17:02:49 +0100") Message-ID: <87y2sijane.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 37305@debbugs.gnu.org Hello Ludovic! Resuming review; I hope I'll be able to bring this series to completion this time. [...] > =E2=80=9Cgnu:=E2=80=9D is for changes to (gnu packages). The idea is tha= t the prefix > should reflect what subsystem the commit is modifying. But yeah, > looking at =E2=80=98git log=E2=80=99 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=E2=80=99s certainly useful to have =E2=80=98--= root=E2=80=99 at least > as an option. > >>> The main issue I see with this change is that mount(2) takes raw strings >>> for the options. There=E2=80=99s a convention to have those strings lo= ok like >>> =E2=80=9CKEY1=3DVALUE1,KEY2=3DVALUE2=E2=80=9D, but it=E2=80=99s just a = convention. >>> >>> As a rule of thumb, I=E2=80=99d 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 =E2=80=9Cdefaults=E2=80=9D option, a way to pass mount(2)= flags as > options (like =E2=80=9Cro=E2=80=9D, =E2=80=9Cremount=E2=80=9D, =E2=80=9Cb= ind=E2=80=9D), /etc/fstab handling, etc. > > Guix System handles /etc/fstab differently and =E2=80=9Cdefaults=E2=80=9D= makes little > sense in our API (one can just use leave the default value of the > =E2=80=98options=E2=80=99 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=E2=80=99t improve clarity and future-proof-ness (what if a file sys= tem has > a =E2=80=9Cbind=E2=80=9D 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=E2=80=99t 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 >>>> 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" k= ernel >>>> argument, and use it when calling `mount-root-file-system'. Update do= c. >>>> * doc/guix.texi (Initial RAM Disk): Document the use of the "--root-op= tions" >>>> 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=E2=80=99d rather avoid it for now. Less code is better. :-) Done! >>> (Also, in hindsight, I think it was a mistake to call them >>> =E2=80=98--something=E2=80=99. Following the common naming convention,= we should rather >>> call these options =E2=80=98gnu.something=E2=80=99.) >> >> Is this convention detailed somewhere? I haven't found it in 'Standards= '. > > It=E2=80=99s a convention of the Linux kernel, I don=E2=80=99t know if it= =E2=80=99s 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