From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Schantl To: development@lists.ipfire.org Subject: Re: [PATCH 05/17] installer: Add code to create a BTRFS subvolume layout. Date: Tue, 19 Mar 2024 21:05:05 +0100 Message-ID: In-Reply-To: <1CB5662A-5B1D-4DEF-A655-EA72B9E6B9F7@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5728061119475292605==" List-Id: --===============5728061119475292605== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Hello, > > > > On 15 Mar 2024, at 19:14, Stefan Schantl > > wrote: > > > > Signed-off-by: Stefan Schantl > > --- > > 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 --===============5728061119475292605==--