From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 05/17] installer: Add code to create a BTRFS subvolume layout. Date: Fri, 22 Mar 2024 16:21:54 +0000 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5899577769990418768==" List-Id: --===============5899577769990418768== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable > On 19 Mar 2024, at 20:05, Stefan Schantl wrot= e: >=20 > Hello, >>=20 >>=20 >>> On 15 Mar 2024, at 19:14, Stefan Schantl >>> wrote: >>>=20 >>> Signed-off-by: Stefan Schantl >>> --- >>> src/installer/hw.c | 70 >>> ++++++++++++++++++++++++++++++++++++++++++++-- >>> src/installer/hw.h | 5 ++++ >>> 2 files changed, 73 insertions(+), 2 deletions(-) >>>=20 >>> 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 @@ >>>=20 >>> #include "hw.h" >>>=20 >>> +// Array which contains the subvolumes which will be created when >>> installing >>> +// IPFire on a BTRFS. >>> +const char* btrfs_subvolumes[7][2] =3D { >>> + {"@root" ,"/"}, >>> + {"@snapshots", "/.snapshots"}, >>> + {"@home", "/home"}, >>> + {"@cache", "/var/cache"}, >>> + {"@lib", "/var/lib"}, >>> + {"@logs", "/var/log"}, >>> + {"@mails", "/var/mail"} >>> +}; >>=20 >> 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. >>=20 >> You should also NULL-terminate the array. >=20 > Thanks for the hint, I'll adjust the code to use it in this way. >=20 >>=20 >>> + >>> +#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]; >>>=20 >>> @@ -805,6 +819,7 @@ int hw_create_partitions(struct hw_destination* >>> dest, const char* output) { >>>=20 >>> static int hw_format_filesystem(const char* path, int fs, const >>> char* output) { >>> char cmd[STRING_SIZE] =3D "\0"; >>> + int r; >>>=20 >>> // Swap >>> if (fs =3D=3D HW_FS_SWAP) { >>> @@ -824,7 +839,9 @@ static int hw_format_filesystem(const char* >>> path, int fs, const char* output) { >>>=20 >>> // BTRFS >>> } else if (fs =3D=3D HW_FS_BTRFS) { >>> - snprintf(cmd, sizeof(cmd), "/usr/bin/mkfs.btrfs -L rootfs -f %s", >>> path); >>> + r =3D hw_create_btrfs_layout(path, output); >>> + >>> + return r; >>>=20 >>> // FAT32 >>> } else if (fs =3D=3D HW_FS_FAT32) { >>> @@ -833,7 +850,7 @@ static int hw_format_filesystem(const char* >>> path, int fs, const char* output) { >>>=20 >>> assert(*cmd); >>>=20 >>> - int r =3D mysystem(output, cmd); >>> + r =3D mysystem(output, cmd); >>>=20 >>> return r; >>> } >>> @@ -870,6 +887,43 @@ int hw_create_filesystems(struct >>> hw_destination* dest, const char* output) { >>> return 0; >>> } >>>=20 >>> +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); >>=20 >> Again, you are using a label here. Is that a requirement? >=20 > 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. >=20 > As it is optional I also can drop the label, your opinion? I would like to keep it consistent. Either remove it from btrfs, or add it to= the others. Since it is not serving any purpose I would rather remove it. >=20 >>=20 >>> + int r =3D mysystem(output, cmd); >>> + >>> + if (r) >>> + return r; >>> + >>> + // We need to mount the FS in order to create any subvolumes. >>> + r =3D hw_mount(path, DESTINATION_MOUNT_PATH, "btrfs", 0); >>> + >>> + if (r) >>> + return r; >>> + >>> + // Loop through the array of subvolumes to create. >>> + for ( int i =3D 0; i < LEN(btrfs_subvolumes); i++ ) { >>=20 >> If you NULL-terminate your array, you won=E2=80=99t need the LEN() macro >> which I personally don=E2=80=99t consider good style. >>=20 >>> + snprintf(subvolume, sizeof(subvolume), "%s", >>> btrfs_subvolumes[i][0]); >>=20 >> I know that the installer isn=E2=80=99t doing that very much, but you shou= ld >> check the return code of snprintf(). >=20 > Thanks I'll add some checking code. >=20 >>=20 >>> + // Call function to create the subvolume >>> + r =3D hw_create_btrfs_subvolume(output, subvolume); >>> + >>> + if (r) >>> + return r; >>> + } >>> + >>> + // Umount the main BTRFS after subvolume creation. >>> + r =3D 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]; >>>=20 >>> @@ -1219,3 +1273,15 @@ int hw_restore_backup(const char* output, >>> const char* backup_path, const char* d >>>=20 >>> 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); >>=20 >> Likewise. >>=20 >>> + >>> + int r =3D 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; >>> }; >>>=20 >>> +extern const char* btrfs_subvolumes[][2]; >>=20 >> Please declare the array statically and you won=E2=80=99t need this. >>=20 >>> + >>> struct hw* hw_init(); >>> void hw_free(struct hw* hw); >>>=20 >>> @@ -143,4 +145,7 @@ int hw_start_networking(const char* output); >>>=20 >>> void hw_sync(); >>>=20 >>> +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 */ >>> --=20 >>> 2.39.2 >>>=20 >>=20 > - Stefan --===============5899577769990418768==--