GNU bug report logs - #26815
[PATCH 0/3] Hybrid UEFI disk image

Previous Next

Full log

Message #34 received at 26815 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 26815 <at> debbugs.gnu.org
Subject: Re: bug#26815: [PATCH 2/3] vm: Support creating FAT partitions.
Date: Sun, 07 May 2017 17:52:43 +0200

[Message part 1 (text/plain, inline)]

Danny Milosavljevic <dannym <at> scratchpost.org> writes:

> Hi Marius,
>
> On Sun,  7 May 2017 16:36:46 +0200
> Marius Bakke <mbakke <at> fastmail.com> wrote:
>> +(define* (create-ext-file-system partition type
>> +                                 #:key label)
>> +  "Create an ext-family filesystem of TYPE on PARTITION.  If LABEL is true,
>> +use that as the volume name."
>> +  (format #t "creating ~a partition...\n" type)
>> +  (apply system* (string-append "mkfs." type)
>> +         "-F" partition
>> +         (if label
>> +             `("-L" ,label)
>> +             '())))
>
> Could you document that the result of the procedure is the system* status?  Also, is that wise?  I think it should instead do the error handling and (error ...) out on error.  It's longer but less surprising.

I had that first, but the error handling was exactly identical, so opted
to just handle it in the caller. It does sound safer to handle errors
there instead of passing system* around though, will do that in lieu of
other comments. 

>> +(define* (create-fat32-file-system partition
>> +                                   #:key label)
>> +  "Create a FAT32 filesystem on PARTITION, which must be at least 32 MiB long.
>> +If LABEL is true, use that as volume name."
>
> Same as above.
>
>>  (define* (format-partition partition type
> [...]
>> +  (define format-procedure
>> +    (cond
>> +     ((string-prefix? "ext" type)
>> +      (create-ext-file-system partition type #:label label))
>> +     ((string-suffix? "fat" type)
>> +      (create-fat32-file-system partition #:label label))
>> +     (else #f)))
>
> "format-procedure" is not actually the procedure, right? It's already the formatting-status ...

Oops, an artifact of rebasing a lot of revisions...

>> +  (if format-procedure
>> +      (match (status:exit-val format-procedure)
>> +        (0 #t)
>> +        (_ (error "Formatting partition failed.")))
>> +      (error "Unsupported file system.")))
>
> status:exit-val will fail when given #f, which it will get in the error case of format-procedure.

Thanks for pointing that out. Will re-submit this patch shortly.

> scheme@(guile-user)> (status:exit-val #f)
> ERROR: In procedure status:exit-val:
> ERROR: Wrong type (expecting exact integer): #f
>
>> +      "nls_iso8859-1"                            ;for `mkfs.fat`, et.al
>
> This adds nls_iso8859-1 unconditionally. OK.

It's required by "dosfstools" which is also added unconditionally.
Not sure how to improve it.

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.