GNU bug report logs - #74837
[PATCH 0/2] Add resize-fs service

Previous Next

Full log

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Richard Sent <richard <at> freakingpenguin.com>
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, 74837 <at> debbugs.gnu.org
Subject: Re: [bug#74837] [PATCH v2 1/2] gnu: services: Add resize-fs-service.
Date: Sat, 14 Dec 2024 16:23:27 +0100

Hello,

Richard Sent <richard <at> freakingpenguin.com> skribis:

> * gnu/services/admin.scm (resize-fs-configuration): New configuration
> type.
> (resize-fs-shepherd-service): New procedure.
> (resize-fs-service-type): New variable.
> * doc/guix.texi (Miscallaneous Services): Document it.
>
> Change-Id: Icae2fefc9a8d936d4c3add47520258b341f689a4

Nice!  Overall LGTM.  Minor comments below.

> +@subsubheading Resize File System service
> +
> +This service type lets you resize a live file system during boot, which
> +can be convenient if a Guix image is flashed on an SD Card (e.g. for an
> +embedded device) or uploaded to a VPS.  In both cases the medium the
> +image will reside upon may be larger than the image you want to produce.
> +
> +For an embedded device booting from an SD card you may use something like:
> +@lisp
> +(service resize-fs-service-type
> +  (resize-fs-configuration
> +    (file-system
> +     (device (file-system-label "root"))
> +     (type "ext4"))))
> +@end lisp

I would avoid abbreviations as usual and go for
‘file-system-resizing-service-type’.  WDYT?

> +Be extra cautious to use the correct device and type.  The service has
> +little error handling of its own and relies on the underlying tools.
> +Wrong use could end in loss of data or the corruption of the operating
> +system.

Maybe wrap this paragraph in “@quotation Warning”.

> +@item @code{file-system} (default: @code{#f}) (type: file-system)
> +The file-system object to resize.  This object must have the device and
                                   ^
Maybe add “(@pxref{File Systems})”.

> +type fields set.  The others are ignored.

“the @code{device} and @code{type} fields set.  Other fields are
ignored.”

> +@item @code{cloud-utils} (default: @code{cloud-utils}) (type: file-like)
> +The cloud-utils package to use.

Maybe add a sentence explaining that ‘cloud-utils’ is used for its
‘growpart’ command.

I wonder if Guile-Parted could be used instead of ‘growpart’ (shouldn’t
be a blocker though).

> +                    (let/ec return
> +                      (guard (c ((and (invoke-error? c)
> +                                      ;; growpart NOCHANGE exits with 1. It is
> +                                      ;; unlikely the partition was resized
> +                                      ;; while the file system was not. Just
> +                                      ;; exit.
> +                                      (equal? (invoke-error-exit-status c) 1))
> +                                 (format (current-error-port)
> +                                         "The device ~a is already resized.~%" device)
> +                                 ;; Must return something or Shepherd considers
> +                                 ;; the service perpetually starting.
> +                                 (return 0)))
> +                        (apply invoke grow-partition-command))
> +                      (apply invoke grow-filesystem-command)))))))))

No need for ‘let/ec’ here, you can just return from the ‘guard’ handler.

The second patch LGTM, though perhaps it should come before this patch
since it fixes something that the resize service needs.

Could you send updated patches?

Thanks!

Ludo’.

Previous Next

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