Re: [PATCH] build: Refactor file system detection logic.

[Ludovic Courtès]

Hi!

David Craven <address@hidden> 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’.