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

[Michael Tremer <michael.tremer@ipfire.org>]

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



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

> +
> +#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?

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

> + // 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
>