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: 5961 bytes --]



> On 19 Mar 2024, at 20:05, Stefan Schantl <stefan.schantl(a)ipfire.org> wrote:
> 
> 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?

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