From: ludo@gnu.org (Ludovic Courtès)
To: David Craven <david@craven.ch>
Cc: guix-devel@gnu.org
Subject: Re: [PATCH] build: Refactor file system detection logic.
Date: Fri, 06 Jan 2017 14:15:58 +0100 [thread overview]
Message-ID: <871swgpbw1.fsf@gnu.org> (raw)
In-Reply-To: <20170106122552.6794-1-david@craven.ch> (David Craven's message of "Fri, 6 Jan 2017 13:25:52 +0100")
Hi!
David Craven <david@craven.ch> skribis:
> * gnu/build/file-systems.scm (read-superblock, bytevector->label): New
> variables.
> (sub-bytevector): Move to general section.
> (ext2-superblock?, ext2-read-superblock): New variables.
> (ext2-superblock-uuid, ext2-superblock-volume-name): Use
> sub-bytevector and bytevector->label.
> (%ext2-sblock-magic, %ext2-sblock-creator-os, %ext2-sblock-uuid,
> %ext2-sblock-volume-name): Inline constants.
> (luks-superblock?, luks-read-header): New variables.
> (%luks-header-size, %luks-magic): Inline.
> (partition-label-predicate, partition-uuid-predicate,
> luks-partition-uuid-predicate): Use functions that are consistently
> prefixed with file system name.
The title should rather start with “file-systems:”.
LGTM! My only comments are about names (naming is hard!). :-)
> +(define (bytevector->label bv)
> + "Return the volume name of SBLOCK as a string of at most 256 characters, or
> +#f if SBLOCK has no volume name."
> + ;; This is a Latin-1, nul-terminated string.
> + (let ((bytes (take-while (negate zero?) (bytevector->u8-list bv))))
> + (if (null? bytes)
> + #f
> + (list->string (map integer->char bytes)))))
I’d call it ‘null-terminated-latin1->string’ (similar to
‘utf8->string’), since it has nothing to do with volume labels per se.
> -(define (read-ext2-superblock device)
> +(define (ext2-read-superblock device)
I’d prefer to keep the previous name, which is more conventional I think
and more readable.
> -(define (read-luks-header file)
Same here.
> +(define luks-partition-uuid-predicate
This one is fine. :-)
Please make sure relevant system tests still pass (for ext2, the ‘basic’
test is enough; for LUKS you have to run ‘encrypted-root-os’.)
Thank you!
Ludo’.
next prev parent reply other threads:[~2017-01-06 13:16 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-06 12:25 [PATCH] build: Refactor file system detection logic David Craven
2017-01-06 13:15 ` Ludovic Courtès [this message]
2017-01-06 13:36 ` David Craven
2017-01-06 14:10 ` David Craven
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=871swgpbw1.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=david@craven.ch \
--cc=guix-devel@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).