Subject: Re: bug#26815: [PATCH 2/3] vm: Support creating FAT partitions.

From: Danny Milosavljevic <[email-masked]>
Date: Sun, 7 May 2017 17:26:56 +0200

Hi Marius,

On Sun,  7 May 2017 16:36:46 +0200
Marius Bakke <[email-masked]> 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.

> +(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 ...

> +  (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.

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.

Otherwise LGTM.