From mboxrd@z Thu Jan  1 00:00:00 1970
From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=)
Subject: Re: [PATCH] build: Refactor file system detection logic.
Date: Fri, 06 Jan 2017 14:15:58 +0100
Message-ID: <871swgpbw1.fsf@gnu.org>
References: <20170106122552.6794-1-david@craven.ch>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Return-path: <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([2001:4830:134:3::10]:37796)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1cPUNJ-0007fs-6i
	for guix-devel@gnu.org; Fri, 06 Jan 2017 08:16:06 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1cPUNE-0003gN-A3
	for guix-devel@gnu.org; Fri, 06 Jan 2017 08:16:05 -0500
In-Reply-To: <20170106122552.6794-1-david@craven.ch> (David Craven's message
	of "Fri, 6 Jan 2017 13:25:52 +0100")
List-Id: "Development of GNU Guix and the GNU System distribution."
	<guix-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/guix-devel/>
List-Post: <mailto:guix-devel@gnu.org>
List-Help: <mailto:guix-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=subscribe>
Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
Sender: "Guix-devel" <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
To: David Craven <david@craven.ch>
Cc: guix-devel@gnu.org

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 =E2=80=9Cfile-systems:=E2=80=9D.

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 character=
s, 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=E2=80=99d call it =E2=80=98null-terminated-latin1->string=E2=80=99 (simil=
ar to
=E2=80=98utf8->string=E2=80=99), since it has nothing to do with volume lab=
els per se.

> -(define (read-ext2-superblock device)
> +(define (ext2-read-superblock device)

I=E2=80=99d 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 =E2=80=98b=
asic=E2=80=99
test is enough; for LUKS you have to run =E2=80=98encrypted-root-os=E2=80=
=99.)

Thank you!

Ludo=E2=80=99.