diff mbox series

[3/3] installer: Bind-mount /sys/firmware/efi/efivars into chroot

Message ID 20211104090554.6510-3-michael.tremer@ipfire.org
State Accepted
Commit ced15fdfaf4b233dd43fc60df5e2d17e13f95637
Headers
Series [1/3] mountkernfs: Mount /sys/firmware/efi/efivars on EFI systems |

Commit Message

Michael Tremer Nov. 4, 2021, 9:05 a.m. UTC 
  Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
---
 src/installer/hw.c   | 113 +++++++++++++++++++++++++++++--------------
 src/installer/hw.h   |   2 +-
 src/installer/main.c |   2 +-
 3 files changed, 78 insertions(+), 39 deletions(-)

Comments

Peter Müller Nov. 17, 2021, 8:11 p.m. UTC | #1 
Hello Michael,

lacking C skills, this one looks good to me, but I am not confident enough to tag it as being reviewed.

Acked-by: Peter Müller <peter.mueller@ipfire.org>

Thanks, and best regards,
Peter Müller

> Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
> ---
>   src/installer/hw.c   | 113 +++++++++++++++++++++++++++++--------------
>   src/installer/hw.h   |   2 +-
>   src/installer/main.c |   2 +-
>   3 files changed, 78 insertions(+), 39 deletions(-)
> 
> diff --git a/src/installer/hw.c b/src/installer/hw.c
> index 71a1f1cce..265df2d8c 100644
> --- a/src/installer/hw.c
> +++ b/src/installer/hw.c
> @@ -46,13 +46,6 @@
>   
>   #include "hw.h"
>   
> -const char* other_filesystems[] = {
> -	"/dev",
> -	"/proc",
> -	"/sys",
> -	NULL
> -};
> -
>   static int system_chroot(const char* output, const char* path, const char* cmd) {
>   	char chroot_cmd[STRING_SIZE];
>   
> @@ -149,14 +142,53 @@ int hw_mount(const char* source, const char* target, const char* fs, int flags)
>   	return mount(source, target, fs, flags, NULL);
>   }
>   
> -int hw_umount(const char* target) {
> -	int r = umount2(target, 0);
> +static int hw_bind_mount(const char* source, const char* prefix) {
> +	if (!source || !prefix) {
> +		errno = EINVAL;
> +		return 1;
> +	}
>   
> -	if (r && errno == EBUSY) {
> -		// Give it a moment to settle
> -		sleep(1);
> +	char target[PATH_MAX];
> +	int r;
> +
> +	// Format target
> +	r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
> +	if (r < 0)
> +		return 1;
>   
> -		r = umount2(target, MNT_FORCE);
> +	// Ensure target exists
> +	mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
> +
> +	return hw_mount(source, target, NULL, MS_BIND);
> +}
> +
> +int hw_umount(const char* source, const char* prefix) {
> +	char target[PATH_MAX];
> +	int r;
> +
> +	if (prefix)
> +		r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
> +	else
> +		r = snprintf(target, sizeof(target) - 1, "%s", source);
> +	if (r < 0)
> +		return r;
> +
> +	// Perform umount
> +	r = umount2(target, 0);
> +	if (r) {
> +		switch (errno) {
> +			// Try again with force if umount wasn't successful
> +			case EBUSY:
> +				sleep(1);
> +
> +				r = umount2(target, MNT_FORCE);
> +				break;
> +
> +			// target wasn't a mountpoint. Ignore.
> +			case EINVAL:
> +				r = 0;
> +				break;
> +		}
>   	}
>   
>   	return r;
> @@ -174,7 +206,7 @@ static int hw_test_source_medium(const char* path) {
>   	ret = access(SOURCE_TEST_FILE, R_OK);
>   
>   	// Umount the test device.
> -	hw_umount(SOURCE_MOUNT_PATH);
> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>   
>   	return ret;
>   }
> @@ -881,20 +913,21 @@ int hw_mount_filesystems(struct hw_destination* dest, const char* prefix) {
>   	}
>   
>   	// bind-mount misc filesystems
> -	char** otherfs = other_filesystems;
> -	while (*otherfs) {
> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs);
> +	r = hw_bind_mount("/dev", prefix);
> +	if (r)
> +		return r;
>   
> -		mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
> -		r = hw_mount(*otherfs, target, NULL, MS_BIND);
> -		if (r) {
> -			hw_umount_filesystems(dest, prefix);
> +	r = hw_bind_mount("/proc", prefix);
> +	if (r)
> +		return r;
>   
> -			return r;
> -		}
> +	r = hw_bind_mount("/sys", prefix);
> +	if (r)
> +		return r;
>   
> -		otherfs++;
> -	}
> +	r = hw_bind_mount("/sys/firmware/efi/efivars", prefix);
> +	if (r && errno != ENOENT)
> +		return r;
>   
>   	return 0;
>   }
> @@ -908,16 +941,14 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>   
>   	// ESP
>   	if (*dest->part_boot_efi) {
> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI);
> -		r = hw_umount(target);
> +		r = hw_umount(HW_PATH_BOOT_EFI, prefix);
>   		if (r)
>   			return -1;
>   	}
>   
>   	// boot
>   	if (*dest->part_boot) {
> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT);
> -		r = hw_umount(target);
> +		r = hw_umount(HW_PATH_BOOT, prefix);
>   		if (r)
>   			return -1;
>   	}
> @@ -928,16 +959,24 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>   	}
>   
>   	// misc filesystems
> -	char** otherfs = other_filesystems;
> -	while (*otherfs) {
> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++);
> -		r = hw_umount(target);
> -		if (r)
> -			return -1;
> -	}
> +	r = hw_umount("/sys/firmware/efi/efivars", prefix);
> +	if (r)
> +		return -1;
> +
> +	r = hw_umount("/sys", prefix);
> +	if (r)
> +		return -1;
> +
> +	r = hw_umount("/proc", prefix);
> +	if (r)
> +		return -1;
> +
> +	r = hw_umount("/dev", prefix);
> +	if (r)
> +		return -1;
>   
>   	// root
> -	r = hw_umount(prefix);
> +	r = hw_umount(prefix, NULL);
>   	if (r)
>   		return -1;
>   
> diff --git a/src/installer/hw.h b/src/installer/hw.h
> index 9fe69271e..b11dfa48f 100644
> --- a/src/installer/hw.h
> +++ b/src/installer/hw.h
> @@ -108,7 +108,7 @@ struct hw* hw_init();
>   void hw_free(struct hw* hw);
>   
>   int hw_mount(const char* source, const char* target, const char* fs, int flags);
> -int hw_umount(const char* target);
> +int hw_umount(const char* source, const char* prefix);
>   
>   char* hw_find_source_medium(struct hw* hw);
>   
> diff --git a/src/installer/main.c b/src/installer/main.c
> index bc0fdaa67..fabc0ef52 100644
> --- a/src/installer/main.c
> +++ b/src/installer/main.c
> @@ -909,7 +909,7 @@ int main(int argc, char *argv[]) {
>   	}
>   
>   	// Umount source drive and eject
> -	hw_umount(SOURCE_MOUNT_PATH);
> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>   
>   	// Free downloaded ISO image
>   	if (strcmp(sourcedrive, SOURCE_TEMPFILE) == 0) {
>
Michael Tremer Nov. 18, 2021, 11:09 a.m. UTC | #2 
Isn’t an Ack a stronger kind of Review?

https://wiki.ipfire.org/devel/git/tags

However, thanks for looking at this.

-Michael

> On 17 Nov 2021, at 20:11, Peter Müller <peter.mueller@ipfire.org> wrote:
> 
> Hello Michael,
> 
> lacking C skills, this one looks good to me, but I am not confident enough to tag it as being reviewed.
> 
> Acked-by: Peter Müller <peter.mueller@ipfire.org>
> 
> Thanks, and best regards,
> Peter Müller
> 
>> Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
>> ---
>>  src/installer/hw.c   | 113 +++++++++++++++++++++++++++++--------------
>>  src/installer/hw.h   |   2 +-
>>  src/installer/main.c |   2 +-
>>  3 files changed, 78 insertions(+), 39 deletions(-)
>> diff --git a/src/installer/hw.c b/src/installer/hw.c
>> index 71a1f1cce..265df2d8c 100644
>> --- a/src/installer/hw.c
>> +++ b/src/installer/hw.c
>> @@ -46,13 +46,6 @@
>>    #include "hw.h"
>>  -const char* other_filesystems[] = {
>> -	"/dev",
>> -	"/proc",
>> -	"/sys",
>> -	NULL
>> -};
>> -
>>  static int system_chroot(const char* output, const char* path, const char* cmd) {
>>  	char chroot_cmd[STRING_SIZE];
>>  @@ -149,14 +142,53 @@ int hw_mount(const char* source, const char* target, const char* fs, int flags)
>>  	return mount(source, target, fs, flags, NULL);
>>  }
>>  -int hw_umount(const char* target) {
>> -	int r = umount2(target, 0);
>> +static int hw_bind_mount(const char* source, const char* prefix) {
>> +	if (!source || !prefix) {
>> +		errno = EINVAL;
>> +		return 1;
>> +	}
>>  -	if (r && errno == EBUSY) {
>> -		// Give it a moment to settle
>> -		sleep(1);
>> +	char target[PATH_MAX];
>> +	int r;
>> +
>> +	// Format target
>> +	r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
>> +	if (r < 0)
>> +		return 1;
>>  -		r = umount2(target, MNT_FORCE);
>> +	// Ensure target exists
>> +	mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
>> +
>> +	return hw_mount(source, target, NULL, MS_BIND);
>> +}
>> +
>> +int hw_umount(const char* source, const char* prefix) {
>> +	char target[PATH_MAX];
>> +	int r;
>> +
>> +	if (prefix)
>> +		r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
>> +	else
>> +		r = snprintf(target, sizeof(target) - 1, "%s", source);
>> +	if (r < 0)
>> +		return r;
>> +
>> +	// Perform umount
>> +	r = umount2(target, 0);
>> +	if (r) {
>> +		switch (errno) {
>> +			// Try again with force if umount wasn't successful
>> +			case EBUSY:
>> +				sleep(1);
>> +
>> +				r = umount2(target, MNT_FORCE);
>> +				break;
>> +
>> +			// target wasn't a mountpoint. Ignore.
>> +			case EINVAL:
>> +				r = 0;
>> +				break;
>> +		}
>>  	}
>>    	return r;
>> @@ -174,7 +206,7 @@ static int hw_test_source_medium(const char* path) {
>>  	ret = access(SOURCE_TEST_FILE, R_OK);
>>    	// Umount the test device.
>> -	hw_umount(SOURCE_MOUNT_PATH);
>> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>>    	return ret;
>>  }
>> @@ -881,20 +913,21 @@ int hw_mount_filesystems(struct hw_destination* dest, const char* prefix) {
>>  	}
>>    	// bind-mount misc filesystems
>> -	char** otherfs = other_filesystems;
>> -	while (*otherfs) {
>> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs);
>> +	r = hw_bind_mount("/dev", prefix);
>> +	if (r)
>> +		return r;
>>  -		mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
>> -		r = hw_mount(*otherfs, target, NULL, MS_BIND);
>> -		if (r) {
>> -			hw_umount_filesystems(dest, prefix);
>> +	r = hw_bind_mount("/proc", prefix);
>> +	if (r)
>> +		return r;
>>  -			return r;
>> -		}
>> +	r = hw_bind_mount("/sys", prefix);
>> +	if (r)
>> +		return r;
>>  -		otherfs++;
>> -	}
>> +	r = hw_bind_mount("/sys/firmware/efi/efivars", prefix);
>> +	if (r && errno != ENOENT)
>> +		return r;
>>    	return 0;
>>  }
>> @@ -908,16 +941,14 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>>    	// ESP
>>  	if (*dest->part_boot_efi) {
>> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI);
>> -		r = hw_umount(target);
>> +		r = hw_umount(HW_PATH_BOOT_EFI, prefix);
>>  		if (r)
>>  			return -1;
>>  	}
>>    	// boot
>>  	if (*dest->part_boot) {
>> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT);
>> -		r = hw_umount(target);
>> +		r = hw_umount(HW_PATH_BOOT, prefix);
>>  		if (r)
>>  			return -1;
>>  	}
>> @@ -928,16 +959,24 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>>  	}
>>    	// misc filesystems
>> -	char** otherfs = other_filesystems;
>> -	while (*otherfs) {
>> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++);
>> -		r = hw_umount(target);
>> -		if (r)
>> -			return -1;
>> -	}
>> +	r = hw_umount("/sys/firmware/efi/efivars", prefix);
>> +	if (r)
>> +		return -1;
>> +
>> +	r = hw_umount("/sys", prefix);
>> +	if (r)
>> +		return -1;
>> +
>> +	r = hw_umount("/proc", prefix);
>> +	if (r)
>> +		return -1;
>> +
>> +	r = hw_umount("/dev", prefix);
>> +	if (r)
>> +		return -1;
>>    	// root
>> -	r = hw_umount(prefix);
>> +	r = hw_umount(prefix, NULL);
>>  	if (r)
>>  		return -1;
>>  diff --git a/src/installer/hw.h b/src/installer/hw.h
>> index 9fe69271e..b11dfa48f 100644
>> --- a/src/installer/hw.h
>> +++ b/src/installer/hw.h
>> @@ -108,7 +108,7 @@ struct hw* hw_init();
>>  void hw_free(struct hw* hw);
>>    int hw_mount(const char* source, const char* target, const char* fs, int flags);
>> -int hw_umount(const char* target);
>> +int hw_umount(const char* source, const char* prefix);
>>    char* hw_find_source_medium(struct hw* hw);
>>  diff --git a/src/installer/main.c b/src/installer/main.c
>> index bc0fdaa67..fabc0ef52 100644
>> --- a/src/installer/main.c
>> +++ b/src/installer/main.c
>> @@ -909,7 +909,7 @@ int main(int argc, char *argv[]) {
>>  	}
>>    	// Umount source drive and eject
>> -	hw_umount(SOURCE_MOUNT_PATH);
>> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>>    	// Free downloaded ISO image
>>  	if (strcmp(sourcedrive, SOURCE_TEMPFILE) == 0) {
Peter Müller Nov. 19, 2021, 6:30 a.m. UTC | #3 
Hello Michael,

interesting. I understood that wiki page the other way; a "looks good to me"
intuitively sounds weaker than "I have reviewed this and am okay with it" to
me. Glad you mentioned this.

Thanks, and best regards,
Peter Müller


> Isn’t an Ack a stronger kind of Review?
> 
> https://wiki.ipfire.org/devel/git/tags
> 
> However, thanks for looking at this.
> 
> -Michael
> 
>> On 17 Nov 2021, at 20:11, Peter Müller <peter.mueller@ipfire.org> wrote:
>>
>> Hello Michael,
>>
>> lacking C skills, this one looks good to me, but I am not confident enough to tag it as being reviewed.
>>
>> Acked-by: Peter Müller <peter.mueller@ipfire.org>
>>
>> Thanks, and best regards,
>> Peter Müller
>>
>>> Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
>>> ---
>>>  src/installer/hw.c   | 113 +++++++++++++++++++++++++++++--------------
>>>  src/installer/hw.h   |   2 +-
>>>  src/installer/main.c |   2 +-
>>>  3 files changed, 78 insertions(+), 39 deletions(-)
>>> diff --git a/src/installer/hw.c b/src/installer/hw.c
>>> index 71a1f1cce..265df2d8c 100644
>>> --- a/src/installer/hw.c
>>> +++ b/src/installer/hw.c
>>> @@ -46,13 +46,6 @@
>>>    #include "hw.h"
>>>  -const char* other_filesystems[] = {
>>> -	"/dev",
>>> -	"/proc",
>>> -	"/sys",
>>> -	NULL
>>> -};
>>> -
>>>  static int system_chroot(const char* output, const char* path, const char* cmd) {
>>>  	char chroot_cmd[STRING_SIZE];
>>>  @@ -149,14 +142,53 @@ int hw_mount(const char* source, const char* target, const char* fs, int flags)
>>>  	return mount(source, target, fs, flags, NULL);
>>>  }
>>>  -int hw_umount(const char* target) {
>>> -	int r = umount2(target, 0);
>>> +static int hw_bind_mount(const char* source, const char* prefix) {
>>> +	if (!source || !prefix) {
>>> +		errno = EINVAL;
>>> +		return 1;
>>> +	}
>>>  -	if (r && errno == EBUSY) {
>>> -		// Give it a moment to settle
>>> -		sleep(1);
>>> +	char target[PATH_MAX];
>>> +	int r;
>>> +
>>> +	// Format target
>>> +	r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
>>> +	if (r < 0)
>>> +		return 1;
>>>  -		r = umount2(target, MNT_FORCE);
>>> +	// Ensure target exists
>>> +	mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
>>> +
>>> +	return hw_mount(source, target, NULL, MS_BIND);
>>> +}
>>> +
>>> +int hw_umount(const char* source, const char* prefix) {
>>> +	char target[PATH_MAX];
>>> +	int r;
>>> +
>>> +	if (prefix)
>>> +		r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
>>> +	else
>>> +		r = snprintf(target, sizeof(target) - 1, "%s", source);
>>> +	if (r < 0)
>>> +		return r;
>>> +
>>> +	// Perform umount
>>> +	r = umount2(target, 0);
>>> +	if (r) {
>>> +		switch (errno) {
>>> +			// Try again with force if umount wasn't successful
>>> +			case EBUSY:
>>> +				sleep(1);
>>> +
>>> +				r = umount2(target, MNT_FORCE);
>>> +				break;
>>> +
>>> +			// target wasn't a mountpoint. Ignore.
>>> +			case EINVAL:
>>> +				r = 0;
>>> +				break;
>>> +		}
>>>  	}
>>>    	return r;
>>> @@ -174,7 +206,7 @@ static int hw_test_source_medium(const char* path) {
>>>  	ret = access(SOURCE_TEST_FILE, R_OK);
>>>    	// Umount the test device.
>>> -	hw_umount(SOURCE_MOUNT_PATH);
>>> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>>>    	return ret;
>>>  }
>>> @@ -881,20 +913,21 @@ int hw_mount_filesystems(struct hw_destination* dest, const char* prefix) {
>>>  	}
>>>    	// bind-mount misc filesystems
>>> -	char** otherfs = other_filesystems;
>>> -	while (*otherfs) {
>>> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs);
>>> +	r = hw_bind_mount("/dev", prefix);
>>> +	if (r)
>>> +		return r;
>>>  -		mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
>>> -		r = hw_mount(*otherfs, target, NULL, MS_BIND);
>>> -		if (r) {
>>> -			hw_umount_filesystems(dest, prefix);
>>> +	r = hw_bind_mount("/proc", prefix);
>>> +	if (r)
>>> +		return r;
>>>  -			return r;
>>> -		}
>>> +	r = hw_bind_mount("/sys", prefix);
>>> +	if (r)
>>> +		return r;
>>>  -		otherfs++;
>>> -	}
>>> +	r = hw_bind_mount("/sys/firmware/efi/efivars", prefix);
>>> +	if (r && errno != ENOENT)
>>> +		return r;
>>>    	return 0;
>>>  }
>>> @@ -908,16 +941,14 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>>>    	// ESP
>>>  	if (*dest->part_boot_efi) {
>>> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI);
>>> -		r = hw_umount(target);
>>> +		r = hw_umount(HW_PATH_BOOT_EFI, prefix);
>>>  		if (r)
>>>  			return -1;
>>>  	}
>>>    	// boot
>>>  	if (*dest->part_boot) {
>>> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT);
>>> -		r = hw_umount(target);
>>> +		r = hw_umount(HW_PATH_BOOT, prefix);
>>>  		if (r)
>>>  			return -1;
>>>  	}
>>> @@ -928,16 +959,24 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>>>  	}
>>>    	// misc filesystems
>>> -	char** otherfs = other_filesystems;
>>> -	while (*otherfs) {
>>> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++);
>>> -		r = hw_umount(target);
>>> -		if (r)
>>> -			return -1;
>>> -	}
>>> +	r = hw_umount("/sys/firmware/efi/efivars", prefix);
>>> +	if (r)
>>> +		return -1;
>>> +
>>> +	r = hw_umount("/sys", prefix);
>>> +	if (r)
>>> +		return -1;
>>> +
>>> +	r = hw_umount("/proc", prefix);
>>> +	if (r)
>>> +		return -1;
>>> +
>>> +	r = hw_umount("/dev", prefix);
>>> +	if (r)
>>> +		return -1;
>>>    	// root
>>> -	r = hw_umount(prefix);
>>> +	r = hw_umount(prefix, NULL);
>>>  	if (r)
>>>  		return -1;
>>>  diff --git a/src/installer/hw.h b/src/installer/hw.h
>>> index 9fe69271e..b11dfa48f 100644
>>> --- a/src/installer/hw.h
>>> +++ b/src/installer/hw.h
>>> @@ -108,7 +108,7 @@ struct hw* hw_init();
>>>  void hw_free(struct hw* hw);
>>>    int hw_mount(const char* source, const char* target, const char* fs, int flags);
>>> -int hw_umount(const char* target);
>>> +int hw_umount(const char* source, const char* prefix);
>>>    char* hw_find_source_medium(struct hw* hw);
>>>  diff --git a/src/installer/main.c b/src/installer/main.c
>>> index bc0fdaa67..fabc0ef52 100644
>>> --- a/src/installer/main.c
>>> +++ b/src/installer/main.c
>>> @@ -909,7 +909,7 @@ int main(int argc, char *argv[]) {
>>>  	}
>>>    	// Umount source drive and eject
>>> -	hw_umount(SOURCE_MOUNT_PATH);
>>> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>>>    	// Free downloaded ISO image
>>>  	if (strcmp(sourcedrive, SOURCE_TEMPFILE) == 0) {
>
Michael Tremer Nov. 19, 2021, 11:10 a.m. UTC | #4 
Hmm, difficult. Reading it again it is a little bit unclear.

I suppose the Ack-by doesn’t quite fit into a bucket like Reported-by and Tested-by which are very clear.

It is indeed maybe a more relaxed review, but with a “I want this change” in it. So it makes sense that you used that.

Never mind me.

-Michael

> On 19 Nov 2021, at 06:30, Peter Müller <peter.mueller@ipfire.org> wrote:
> 
> Hello Michael,
> 
> interesting. I understood that wiki page the other way; a "looks good to me"
> intuitively sounds weaker than "I have reviewed this and am okay with it" to
> me. Glad you mentioned this.
> 
> Thanks, and best regards,
> Peter Müller
> 
> 
>> Isn’t an Ack a stronger kind of Review?
>> 
>> https://wiki.ipfire.org/devel/git/tags
>> 
>> However, thanks for looking at this.
>> 
>> -Michael
>> 
>>> On 17 Nov 2021, at 20:11, Peter Müller <peter.mueller@ipfire.org> wrote:
>>> 
>>> Hello Michael,
>>> 
>>> lacking C skills, this one looks good to me, but I am not confident enough to tag it as being reviewed.
>>> 
>>> Acked-by: Peter Müller <peter.mueller@ipfire.org>
>>> 
>>> Thanks, and best regards,
>>> Peter Müller
>>> 
>>>> Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
>>>> ---
>>>> src/installer/hw.c   | 113 +++++++++++++++++++++++++++++--------------
>>>> src/installer/hw.h   |   2 +-
>>>> src/installer/main.c |   2 +-
>>>> 3 files changed, 78 insertions(+), 39 deletions(-)
>>>> diff --git a/src/installer/hw.c b/src/installer/hw.c
>>>> index 71a1f1cce..265df2d8c 100644
>>>> --- a/src/installer/hw.c
>>>> +++ b/src/installer/hw.c
>>>> @@ -46,13 +46,6 @@
>>>>   #include "hw.h"
>>>> -const char* other_filesystems[] = {
>>>> -	"/dev",
>>>> -	"/proc",
>>>> -	"/sys",
>>>> -	NULL
>>>> -};
>>>> -
>>>> static int system_chroot(const char* output, const char* path, const char* cmd) {
>>>> 	char chroot_cmd[STRING_SIZE];
>>>> @@ -149,14 +142,53 @@ int hw_mount(const char* source, const char* target, const char* fs, int flags)
>>>> 	return mount(source, target, fs, flags, NULL);
>>>> }
>>>> -int hw_umount(const char* target) {
>>>> -	int r = umount2(target, 0);
>>>> +static int hw_bind_mount(const char* source, const char* prefix) {
>>>> +	if (!source || !prefix) {
>>>> +		errno = EINVAL;
>>>> +		return 1;
>>>> +	}
>>>> -	if (r && errno == EBUSY) {
>>>> -		// Give it a moment to settle
>>>> -		sleep(1);
>>>> +	char target[PATH_MAX];
>>>> +	int r;
>>>> +
>>>> +	// Format target
>>>> +	r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
>>>> +	if (r < 0)
>>>> +		return 1;
>>>> -		r = umount2(target, MNT_FORCE);
>>>> +	// Ensure target exists
>>>> +	mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
>>>> +
>>>> +	return hw_mount(source, target, NULL, MS_BIND);
>>>> +}
>>>> +
>>>> +int hw_umount(const char* source, const char* prefix) {
>>>> +	char target[PATH_MAX];
>>>> +	int r;
>>>> +
>>>> +	if (prefix)
>>>> +		r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
>>>> +	else
>>>> +		r = snprintf(target, sizeof(target) - 1, "%s", source);
>>>> +	if (r < 0)
>>>> +		return r;
>>>> +
>>>> +	// Perform umount
>>>> +	r = umount2(target, 0);
>>>> +	if (r) {
>>>> +		switch (errno) {
>>>> +			// Try again with force if umount wasn't successful
>>>> +			case EBUSY:
>>>> +				sleep(1);
>>>> +
>>>> +				r = umount2(target, MNT_FORCE);
>>>> +				break;
>>>> +
>>>> +			// target wasn't a mountpoint. Ignore.
>>>> +			case EINVAL:
>>>> +				r = 0;
>>>> +				break;
>>>> +		}
>>>> 	}
>>>>   	return r;
>>>> @@ -174,7 +206,7 @@ static int hw_test_source_medium(const char* path) {
>>>> 	ret = access(SOURCE_TEST_FILE, R_OK);
>>>>   	// Umount the test device.
>>>> -	hw_umount(SOURCE_MOUNT_PATH);
>>>> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>>>>   	return ret;
>>>> }
>>>> @@ -881,20 +913,21 @@ int hw_mount_filesystems(struct hw_destination* dest, const char* prefix) {
>>>> 	}
>>>>   	// bind-mount misc filesystems
>>>> -	char** otherfs = other_filesystems;
>>>> -	while (*otherfs) {
>>>> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs);
>>>> +	r = hw_bind_mount("/dev", prefix);
>>>> +	if (r)
>>>> +		return r;
>>>> -		mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
>>>> -		r = hw_mount(*otherfs, target, NULL, MS_BIND);
>>>> -		if (r) {
>>>> -			hw_umount_filesystems(dest, prefix);
>>>> +	r = hw_bind_mount("/proc", prefix);
>>>> +	if (r)
>>>> +		return r;
>>>> -			return r;
>>>> -		}
>>>> +	r = hw_bind_mount("/sys", prefix);
>>>> +	if (r)
>>>> +		return r;
>>>> -		otherfs++;
>>>> -	}
>>>> +	r = hw_bind_mount("/sys/firmware/efi/efivars", prefix);
>>>> +	if (r && errno != ENOENT)
>>>> +		return r;
>>>>   	return 0;
>>>> }
>>>> @@ -908,16 +941,14 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>>>>   	// ESP
>>>> 	if (*dest->part_boot_efi) {
>>>> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI);
>>>> -		r = hw_umount(target);
>>>> +		r = hw_umount(HW_PATH_BOOT_EFI, prefix);
>>>> 		if (r)
>>>> 			return -1;
>>>> 	}
>>>>   	// boot
>>>> 	if (*dest->part_boot) {
>>>> -		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT);
>>>> -		r = hw_umount(target);
>>>> +		r = hw_umount(HW_PATH_BOOT, prefix);
>>>> 		if (r)
>>>> 			return -1;
>>>> 	}
>>>> @@ -928,16 +959,24 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
>>>> 	}
>>>>   	// misc filesystems
>>>> -	char** otherfs = other_filesystems;
>>>> -	while (*otherfs) {
>>>> -		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++);
>>>> -		r = hw_umount(target);
>>>> -		if (r)
>>>> -			return -1;
>>>> -	}
>>>> +	r = hw_umount("/sys/firmware/efi/efivars", prefix);
>>>> +	if (r)
>>>> +		return -1;
>>>> +
>>>> +	r = hw_umount("/sys", prefix);
>>>> +	if (r)
>>>> +		return -1;
>>>> +
>>>> +	r = hw_umount("/proc", prefix);
>>>> +	if (r)
>>>> +		return -1;
>>>> +
>>>> +	r = hw_umount("/dev", prefix);
>>>> +	if (r)
>>>> +		return -1;
>>>>   	// root
>>>> -	r = hw_umount(prefix);
>>>> +	r = hw_umount(prefix, NULL);
>>>> 	if (r)
>>>> 		return -1;
>>>> diff --git a/src/installer/hw.h b/src/installer/hw.h
>>>> index 9fe69271e..b11dfa48f 100644
>>>> --- a/src/installer/hw.h
>>>> +++ b/src/installer/hw.h
>>>> @@ -108,7 +108,7 @@ struct hw* hw_init();
>>>> void hw_free(struct hw* hw);
>>>>   int hw_mount(const char* source, const char* target, const char* fs, int flags);
>>>> -int hw_umount(const char* target);
>>>> +int hw_umount(const char* source, const char* prefix);
>>>>   char* hw_find_source_medium(struct hw* hw);
>>>> diff --git a/src/installer/main.c b/src/installer/main.c
>>>> index bc0fdaa67..fabc0ef52 100644
>>>> --- a/src/installer/main.c
>>>> +++ b/src/installer/main.c
>>>> @@ -909,7 +909,7 @@ int main(int argc, char *argv[]) {
>>>> 	}
>>>>   	// Umount source drive and eject
>>>> -	hw_umount(SOURCE_MOUNT_PATH);
>>>> +	hw_umount(SOURCE_MOUNT_PATH, NULL);
>>>>   	// Free downloaded ISO image
>>>> 	if (strcmp(sourcedrive, SOURCE_TEMPFILE) == 0) {
>>
diff mbox series

Patch

diff --git a/src/installer/hw.c b/src/installer/hw.c
index 71a1f1cce..265df2d8c 100644
--- a/src/installer/hw.c
+++ b/src/installer/hw.c
@@ -46,13 +46,6 @@ 
 
 #include "hw.h"
 
-const char* other_filesystems[] = {
-	"/dev",
-	"/proc",
-	"/sys",
-	NULL
-};
-
 static int system_chroot(const char* output, const char* path, const char* cmd) {
 	char chroot_cmd[STRING_SIZE];
 
@@ -149,14 +142,53 @@  int hw_mount(const char* source, const char* target, const char* fs, int flags)
 	return mount(source, target, fs, flags, NULL);
 }
 
-int hw_umount(const char* target) {
-	int r = umount2(target, 0);
+static int hw_bind_mount(const char* source, const char* prefix) {
+	if (!source || !prefix) {
+		errno = EINVAL;
+		return 1;
+	}
 
-	if (r && errno == EBUSY) {
-		// Give it a moment to settle
-		sleep(1);
+	char target[PATH_MAX];
+	int r;
+
+	// Format target
+	r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
+	if (r < 0)
+		return 1;
 
-		r = umount2(target, MNT_FORCE);
+	// Ensure target exists
+	mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
+
+	return hw_mount(source, target, NULL, MS_BIND);
+}
+
+int hw_umount(const char* source, const char* prefix) {
+	char target[PATH_MAX];
+	int r;
+
+	if (prefix)
+		r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
+	else
+		r = snprintf(target, sizeof(target) - 1, "%s", source);
+	if (r < 0)
+		return r;
+
+	// Perform umount
+	r = umount2(target, 0);
+	if (r) {
+		switch (errno) {
+			// Try again with force if umount wasn't successful
+			case EBUSY:
+				sleep(1);
+
+				r = umount2(target, MNT_FORCE);
+				break;
+
+			// target wasn't a mountpoint. Ignore.
+			case EINVAL:
+				r = 0;
+				break;
+		}
 	}
 
 	return r;
@@ -174,7 +206,7 @@  static int hw_test_source_medium(const char* path) {
 	ret = access(SOURCE_TEST_FILE, R_OK);
 
 	// Umount the test device.
-	hw_umount(SOURCE_MOUNT_PATH);
+	hw_umount(SOURCE_MOUNT_PATH, NULL);
 
 	return ret;
 }
@@ -881,20 +913,21 @@  int hw_mount_filesystems(struct hw_destination* dest, const char* prefix) {
 	}
 
 	// bind-mount misc filesystems
-	char** otherfs = other_filesystems;
-	while (*otherfs) {
-		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs);
+	r = hw_bind_mount("/dev", prefix);
+	if (r)
+		return r;
 
-		mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
-		r = hw_mount(*otherfs, target, NULL, MS_BIND);
-		if (r) {
-			hw_umount_filesystems(dest, prefix);
+	r = hw_bind_mount("/proc", prefix);
+	if (r)
+		return r;
 
-			return r;
-		}
+	r = hw_bind_mount("/sys", prefix);
+	if (r)
+		return r;
 
-		otherfs++;
-	}
+	r = hw_bind_mount("/sys/firmware/efi/efivars", prefix);
+	if (r && errno != ENOENT)
+		return r;
 
 	return 0;
 }
@@ -908,16 +941,14 @@  int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
 
 	// ESP
 	if (*dest->part_boot_efi) {
-		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI);
-		r = hw_umount(target);
+		r = hw_umount(HW_PATH_BOOT_EFI, prefix);
 		if (r)
 			return -1;
 	}
 
 	// boot
 	if (*dest->part_boot) {
-		snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT);
-		r = hw_umount(target);
+		r = hw_umount(HW_PATH_BOOT, prefix);
 		if (r)
 			return -1;
 	}
@@ -928,16 +959,24 @@  int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) {
 	}
 
 	// misc filesystems
-	char** otherfs = other_filesystems;
-	while (*otherfs) {
-		snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++);
-		r = hw_umount(target);
-		if (r)
-			return -1;
-	}
+	r = hw_umount("/sys/firmware/efi/efivars", prefix);
+	if (r)
+		return -1;
+
+	r = hw_umount("/sys", prefix);
+	if (r)
+		return -1;
+
+	r = hw_umount("/proc", prefix);
+	if (r)
+		return -1;
+
+	r = hw_umount("/dev", prefix);
+	if (r)
+		return -1;
 
 	// root
-	r = hw_umount(prefix);
+	r = hw_umount(prefix, NULL);
 	if (r)
 		return -1;
 
diff --git a/src/installer/hw.h b/src/installer/hw.h
index 9fe69271e..b11dfa48f 100644
--- a/src/installer/hw.h
+++ b/src/installer/hw.h
@@ -108,7 +108,7 @@  struct hw* hw_init();
 void hw_free(struct hw* hw);
 
 int hw_mount(const char* source, const char* target, const char* fs, int flags);
-int hw_umount(const char* target);
+int hw_umount(const char* source, const char* prefix);
 
 char* hw_find_source_medium(struct hw* hw);
 
diff --git a/src/installer/main.c b/src/installer/main.c
index bc0fdaa67..fabc0ef52 100644
--- a/src/installer/main.c
+++ b/src/installer/main.c
@@ -909,7 +909,7 @@  int main(int argc, char *argv[]) {
 	}
 
 	// Umount source drive and eject
-	hw_umount(SOURCE_MOUNT_PATH);
+	hw_umount(SOURCE_MOUNT_PATH, NULL);
 
 	// Free downloaded ISO image
 	if (strcmp(sourcedrive, SOURCE_TEMPFILE) == 0) {