Re: [PATCH 05/17] installer: Add code to create a BTRFS subvolume layout.

[Stefan Schantl <stefan.schantl@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 5450 bytes --]

Hello,
> 
> 
> > On 15 Mar 2024, at 19:14, Stefan Schantl
> > <stefan.schantl(a)ipfire.org> wrote:
> > 
> > Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> > ---
> > src/installer/hw.c | 70
> > ++++++++++++++++++++++++++++++++++++++++++++--
> > src/installer/hw.h |  5 ++++
> > 2 files changed, 73 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/installer/hw.c b/src/installer/hw.c
> > index 81be87471..420feaca7 100644
> > --- a/src/installer/hw.c
> > +++ b/src/installer/hw.c
> > @@ -44,6 +44,20 @@
> > 
> > #include "hw.h"
> > 
> > +// Array which contains the subvolumes which will be created when
> > installing
> > +// IPFire on a BTRFS.
> > +const char* btrfs_subvolumes[7][2] = {
> > + {"@root" ,"/"},
> > + {"@snapshots", "/.snapshots"},
> > + {"@home", "/home"},
> > + {"@cache", "/var/cache"},
> > + {"@lib", "/var/lib"},
> > + {"@logs", "/var/log"},
> > + {"@mails", "/var/mail"}
> > +};
> 
> I would prefer to not allocate these arrays to a specific size. You
> leave the square brackets empty and the compiler will figure out how
> much space it needs. That avoids any problems when something is being
> added later.
> 
> You should also NULL-terminate the array.

Thanks for the hint, I'll adjust the code to use it in this way.

> 
> > +
> > +#define LEN(arr) ((int) (sizeof (arr) / sizeof (arr)[0]))
> > +
> > static int system_chroot(const char* output, const char* path,
> > const char* cmd) {
> > char chroot_cmd[STRING_SIZE];
> > 
> > @@ -805,6 +819,7 @@ int hw_create_partitions(struct hw_destination*
> > dest, const char* output) {
> > 
> > static int hw_format_filesystem(const char* path, int fs, const
> > char* output) {
> > char cmd[STRING_SIZE] = "\0";
> > + int r;
> > 
> > // Swap
> > if (fs == HW_FS_SWAP) {
> > @@ -824,7 +839,9 @@ static int hw_format_filesystem(const char*
> > path, int fs, const char* output) {
> > 
> > // BTRFS
> > } else if (fs == HW_FS_BTRFS) {
> > - snprintf(cmd, sizeof(cmd), "/usr/bin/mkfs.btrfs -L rootfs -f %s",
> > path);
> > + r = hw_create_btrfs_layout(path, output);
> > +
> > + return r;
> > 
> > // FAT32
> > } else if (fs == HW_FS_FAT32) {
> > @@ -833,7 +850,7 @@ static int hw_format_filesystem(const char*
> > path, int fs, const char* output) {
> > 
> > assert(*cmd);
> > 
> > - int r = mysystem(output, cmd);
> > + r = mysystem(output, cmd);
> > 
> > return r;
> > }
> > @@ -870,6 +887,43 @@ int hw_create_filesystems(struct
> > hw_destination* dest, const char* output) {
> > return 0;
> > }
> > 
> > +int hw_create_btrfs_layout(const char* path, const char* output) {
> > + char cmd[STRING_SIZE];
> > + char subvolume[STRING_SIZE];
> > +
> > + // Create the main BTRFS.
> > + snprintf(cmd, sizeof(cmd), "/usr/bin/mkfs.btrfs -L IPFire -f %s",
> > path);
> 
> Again, you are using a label here. Is that a requirement?

There is not a strict requirement on using a label. I added one as some
kind of nice to have to indicate the main IPFire file system.

As it is optional I also can drop the label, your opinion?

> 
> > + int r = mysystem(output, cmd);
> > +
> > + if (r)
> > + return r;
> > +
> > + // We need to mount the FS in order to create any subvolumes.
> > + r = hw_mount(path, DESTINATION_MOUNT_PATH, "btrfs", 0);
> > +
> > + if (r)
> > + return r;
> > +
> > + // Loop through the array of subvolumes to create.
> > + for ( int i = 0; i < LEN(btrfs_subvolumes); i++ ) {
> 
> If you NULL-terminate your array, you won’t need the LEN() macro
> which I personally don’t consider good style.
> 
> > + snprintf(subvolume, sizeof(subvolume), "%s",
> > btrfs_subvolumes[i][0]);
> 
> I know that the installer isn’t doing that very much, but you should
> check the return code of snprintf().

Thanks I'll add some checking code.

> 
> > + // Call function to create the subvolume
> > + r = hw_create_btrfs_subvolume(output, subvolume);
> > +
> > + if (r)
> > + return r;
> > + }
> > +
> > + // Umount the main BTRFS after subvolume creation.
> > + r = hw_umount(path, 0);
> > +
> > + if (r)
> > + return r;
> > +
> > + return 0;
> > +}
> > +
> > int hw_mount_filesystems(struct hw_destination* dest, const char*
> > prefix) {
> > char target[STRING_SIZE];
> > 
> > @@ -1219,3 +1273,15 @@ int hw_restore_backup(const char* output,
> > const char* backup_path, const char* d
> > 
> > return 0;
> > }
> > +
> > +int hw_create_btrfs_subvolume(const char* output, const char*
> > subvolume) {
> > + char command [STRING_SIZE];
> > + snprintf(command, sizeof(command), "/usr/bin/btrfs subvolume
> > create  %s/%s", DESTINATION_MOUNT_PATH, subvolume);
> 
> Likewise.
> 
> > +
> > + int r = mysystem(output, command);
> > +
> > + if (r)
> > + return -1;
> > +
> > + return 0;
> > +}
> > diff --git a/src/installer/hw.h b/src/installer/hw.h
> > index e5ee65a6d..2de73a3be 100644
> > --- a/src/installer/hw.h
> > +++ b/src/installer/hw.h
> > @@ -104,6 +104,8 @@ struct hw_destination {
> > unsigned long long size_root;
> > };
> > 
> > +extern const char* btrfs_subvolumes[][2];
> 
> Please declare the array statically and you won’t need this.
> 
> > +
> > struct hw* hw_init();
> > void hw_free(struct hw* hw);
> > 
> > @@ -143,4 +145,7 @@ int hw_start_networking(const char* output);
> > 
> > void hw_sync();
> > 
> > +int hw_create_btrfs_layout(const char* output, const char*
> > subvolume);
> > +int hw_create_btrfs_subvolume(const char* output, const char*
> > subvolume);
> > +
> > #endif /* HEADER_HW_H */
> > -- 
> > 2.39.2
> > 
> 
- Stefan