From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 3/3] installer: Bind-mount /sys/firmware/efi/efivars into chroot Date: Thu, 18 Nov 2021 11:09:50 +0000 Message-ID: <6902D37B-36CE-4C3F-B902-93D815790214@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7669480457691629296==" List-Id: --===============7669480457691629296== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Isn=E2=80=99t 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=C3=BCller wr= ote: >=20 > Hello Michael, >=20 > lacking C skills, this one looks good to me, but I am not confident enough = to tag it as being reviewed. >=20 > Acked-by: Peter M=C3=BCller >=20 > Thanks, and best regards, > Peter M=C3=BCller >=20 >> Signed-off-by: Michael Tremer >> --- >> 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[] =3D { >> - "/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 =3D umount2(target, 0); >> +static int hw_bind_mount(const char* source, const char* prefix) { >> + if (!source || !prefix) { >> + errno =3D EINVAL; >> + return 1; >> + } >> - if (r && errno =3D=3D EBUSY) { >> - // Give it a moment to settle >> - sleep(1); >> + char target[PATH_MAX]; >> + int r; >> + >> + // Format target >> + r =3D snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source); >> + if (r < 0) >> + return 1; >> - r =3D 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 =3D snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source); >> + else >> + r =3D snprintf(target, sizeof(target) - 1, "%s", source); >> + if (r < 0) >> + return r; >> + >> + // Perform umount >> + r =3D umount2(target, 0); >> + if (r) { >> + switch (errno) { >> + // Try again with force if umount wasn't successful >> + case EBUSY: >> + sleep(1); >> + >> + r =3D umount2(target, MNT_FORCE); >> + break; >> + >> + // target wasn't a mountpoint. Ignore. >> + case EINVAL: >> + r =3D 0; >> + break; >> + } >> } >> return r; >> @@ -174,7 +206,7 @@ static int hw_test_source_medium(const char* path) { >> ret =3D 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 =3D other_filesystems; >> - while (*otherfs) { >> - snprintf(target, sizeof(target), "%s%s", prefix, *otherfs); >> + r =3D hw_bind_mount("/dev", prefix); >> + if (r) >> + return r; >> - mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO); >> - r =3D hw_mount(*otherfs, target, NULL, MS_BIND); >> - if (r) { >> - hw_umount_filesystems(dest, prefix); >> + r =3D hw_bind_mount("/proc", prefix); >> + if (r) >> + return r; >> - return r; >> - } >> + r =3D hw_bind_mount("/sys", prefix); >> + if (r) >> + return r; >> - otherfs++; >> - } >> + r =3D hw_bind_mount("/sys/firmware/efi/efivars", prefix); >> + if (r && errno !=3D ENOENT) >> + return r; >> return 0; >> } >> @@ -908,16 +941,14 @@ int hw_umount_filesystems(struct hw_destination* des= t, const char* prefix) { >> // ESP >> if (*dest->part_boot_efi) { >> - snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI); >> - r =3D hw_umount(target); >> + r =3D 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 =3D hw_umount(target); >> + r =3D hw_umount(HW_PATH_BOOT, prefix); >> if (r) >> return -1; >> } >> @@ -928,16 +959,24 @@ int hw_umount_filesystems(struct hw_destination* des= t, const char* prefix) { >> } >> // misc filesystems >> - char** otherfs =3D other_filesystems; >> - while (*otherfs) { >> - snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++); >> - r =3D hw_umount(target); >> - if (r) >> - return -1; >> - } >> + r =3D hw_umount("/sys/firmware/efi/efivars", prefix); >> + if (r) >> + return -1; >> + >> + r =3D hw_umount("/sys", prefix); >> + if (r) >> + return -1; >> + >> + r =3D hw_umount("/proc", prefix); >> + if (r) >> + return -1; >> + >> + r =3D hw_umount("/dev", prefix); >> + if (r) >> + return -1; >> // root >> - r =3D hw_umount(prefix); >> + r =3D 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, in= t 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) =3D=3D 0) { --===============7669480457691629296==--