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

["Peter Müller" <peter.mueller@ipfire.org>]

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

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(a)ipfire.org>

Thanks, and best regards,
Peter Müller

> Signed-off-by: Michael Tremer <michael.tremer(a)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) {
>