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

  Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org>
---
 src/installer/hw.c | 70 ++++++++++++++++++++++++++++++++++++++++++++--
 src/installer/hw.h |  5 ++++
 2 files changed, 73 insertions(+), 2 deletions(-)
  

> On 15 Mar 2024, at 19:14, Stefan Schantl <stefan.schantl@ipfire.org> wrote:
> 
> Signed-off-by: Stefan Schantl <stefan.schantl@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
>
  

Hello,
> 
> 
> > On 15 Mar 2024, at 19:14, Stefan Schantl
> > <stefan.schantl@ipfire.org> wrote:
> > 
> > Signed-off-by: Stefan Schantl <stefan.schantl@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
  

> On 19 Mar 2024, at 20:05, Stefan Schantl <stefan.schantl@ipfire.org> wrote:
> 
> Hello,
>> 
>> 
>>> On 15 Mar 2024, at 19:14, Stefan Schantl
>>> <stefan.schantl@ipfire.org> wrote:
>>> 
>>> Signed-off-by: Stefan Schantl <stefan.schantl@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?

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.

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