From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: 40998@debbugs.gnu.org
Subject: bug#40998: Guix System's initrd doesn't honor rootflags
Date: Mon, 28 Feb 2022 16:45:40 -0500 [thread overview]
Message-ID: <87mtiasm0r.fsf@gmail.com> (raw)
In-Reply-To: <87fso2bsm2.fsf@gnu.org> ("Ludovic Courtès"'s message of "Mon, 28 Feb 2022 22:15:17 +0100")
Hi Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> There’s no need to have a ‘version’ field in live <boot-parameters>
>>> records: have the ‘version’ field in the serialized format (the sexp)
>>> and make sure the deserializer correctly converts to the internal
>>> representation.
>>>
>>> Here, I think you can bump the version number in the serialized form,
>>> and have ‘read-boot-parameters’ automatically augment ‘kernel-arguments’
>>> when VERSION is 0 with “--root=XYZ”.
>>
>> I initially went that route, as this was the idea you'd given me
>> initially. However, 'read-boot-parameters' deals with serializing the
>> parameters file only; to add 'root', 'gnu.load' and 'gnu.system', the
>> operating-system object as well as the root device are needed.
>
> <boot-parameters> already has ‘root-device’, so that’s fine.
>
> But you’re right that the system itself is a problem because that’s
> self-referential—it’s the thing the “parameters” file is in. Hmm!
>
> We could add a substitution mechanism where a literal “$SYSTEM” (say) in
> the ‘kernel-arguments’ of <boot-parameters> would be substituted by the
> actual system store file name by ‘read-boot-parameters’, but maybe
> that’s overkill.
>
> So long story short: keeping the ‘version’ field in <boot-parameters>
> sounds reasonable after all. :-)
OK, good, thanks for having confirmed that.
>> The reason 'gnu.load' and 'gnu.system' aren't written to the
>> parameters file to start with is because they would cause the system
>> directory to no longer be content-addressable; this is explained in
>> the docstring of 'operating-system-boot-parameters-file':
>>
>> When SYSTEM-KERNEL-ARGUMENTS? is true, add kernel arguments such as 'root'
>> and 'gnu.load' to the returned file (since the returned file is then usually
>> stored into the content-addressed "system" directory, it's usually not a
>> good idea to give it because the content hash would change by the content hash
>> being stored into the "parameters" file).
>
> This comment originates in 40fad1c24ce60076e26f6dc8096e4716d31d90c3. I
> find it a bit misleading because nothing’s “content-addressed”, but I
> guess it refers to the same problem: that this is self-referential.
>
> (There’s only one use of #:system-kernel-arguments? #t. We can remove
> that keyword parameter from ‘operating-system-boot-parameters-file’
> since it’s not used there.)
OK, I'll address this confusing comment and extraneous argument in a
separate commit.
>>> Also, you could write the ‘match’ pattern like this:
>>>
>>> ('boot-parameters ('version (and version (or 0 1)))
>>> ('label label) …)
>>
>> I think this patch's current form is preferable, as it means future
>> boot-parameters version bumps won't break older Guices (when
>> reconfiguring), as long as the version is an exact, non-negative integer
>> (e.g. when going from 1 to 2).
>
> That’s what we want to avoid: bumping the version number means that the
> new format is not backwards-compatible, and that older Guix versions
> won’t be able to read it. That’s why I think ‘read-boot-parameters’
> needs to be explicit about the version(s) it expects. (A more complete
> example of this pattern is ‘sexp->manifest’.)
>
> Breaking backwards compatibility should be avoided when possible, but
> it’s not always possible. In ‘read-boot-parameters’, ‘bootloader-name’,
> ‘bootloader-menu-entries’, ‘kernel’, etc. are handled somewhat weirdly
> to preserve backwards-compatibility; doing this allowed us to not bump
> the file format version.
Thanks for explaining! I initially thought the choice to break backward
compatibility could be left to the implementer of a new version, but now
I see this wouldn't work or be brittle.
I've modified the first commit like so:
--8<---------------cut here---------------start------------->8---
modified gnu/system.scm
@@ -365,8 +365,10 @@ (define uuid-sexp->uuid
(warning (G_ "unrecognized uuid ~a at '~a'~%") x (port-filename port))
#f)))
+ ;; New versions are not backward-compatible, so only accept past and current
+ ;; versions, not future ones.
(define (version? n)
- (and (exact-integer? n) (not (negative? n))))
+ (member n (iota (1+ %boot-parameters-version))))
(match (read port)
(('boot-parameters ('version (? version? version))
--8<---------------cut here---------------end--------------->8---
So that there's only one thing to update when we'll bump the version
again in the future (%boot-parameters-version).
I'll be sending a v3 shortly, thanks again!
Maxim
prev parent reply other threads:[~2022-02-28 21:46 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-01 13:53 bug#40998: Guix System's initrd doesn't honor rootflags maxim.cournoyer
2022-02-18 15:33 ` bug#40998: [PATCH 1/3] initrd: Rename the '--root' initrd option to just 'root' Maxim Cournoyer
2022-02-18 15:33 ` bug#40998: [PATCH 2/3] initrd: Honor rootfstype and rootflags command-line parameters Maxim Cournoyer
2022-02-18 15:33 ` bug#40998: [PATCH 3/3] initrd: Print its " Maxim Cournoyer
2022-02-19 7:01 ` bug#40998: [PATCH v2 1/4] system: Add a version field to the <boot-parameters> record Maxim Cournoyer
2022-02-19 7:01 ` bug#40998: [PATCH v2 2/4] initrd: Use non-hyphenated kernel command-line parameter names Maxim Cournoyer
2022-02-27 21:03 ` bug#40998: Guix System's initrd doesn't honor rootflags Ludovic Courtès
2022-02-28 20:31 ` Maxim Cournoyer
2022-02-19 7:01 ` bug#40998: [PATCH v2 3/4] initrd: Honor rootfstype and rootflags command-line parameters Maxim Cournoyer
2022-02-27 21:06 ` bug#40998: Guix System's initrd doesn't honor rootflags Ludovic Courtès
2022-02-19 7:01 ` bug#40998: [PATCH v2 4/4] initrd: Print its command-line parameters Maxim Cournoyer
2022-02-27 21:08 ` bug#40998: Guix System's initrd doesn't honor rootflags Ludovic Courtès
2022-02-28 20:36 ` Maxim Cournoyer
2022-02-28 3:45 ` bug#40998: [PATCH v2 4/4] initrd: Print its command-line parameters Tobias Geerinckx-Rice via Bug reports for GNU Guix
2022-02-28 15:54 ` Maxim Cournoyer
2022-02-27 20:54 ` bug#40998: Guix System's initrd doesn't honor rootflags Ludovic Courtès
2022-02-28 20:02 ` Maxim Cournoyer
2022-02-28 21:15 ` Ludovic Courtès
2022-02-28 21:45 ` Maxim Cournoyer [this message]
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=87mtiasm0r.fsf@gmail.com \
--to=maxim.cournoyer@gmail.com \
--cc=40998@debbugs.gnu.org \
--cc=ludo@gnu.org \
/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).