From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter =?utf-8?q?M=C3=BCller?= To: development@lists.ipfire.org Subject: Re: [PATCH 3/3] installer: Bind-mount /sys/firmware/efi/efivars into chroot Date: Fri, 19 Nov 2021 07:30:09 +0100 Message-ID: In-Reply-To: <6902D37B-36CE-4C3F-B902-93D815790214@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4073372293091193843==" List-Id: --===============4073372293091193843== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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=C3=BCller > Isn=E2=80=99t an Ack a stronger kind of Review? >=20 > https://wiki.ipfire.org/devel/git/tags >=20 > However, thanks for looking at this. >=20 > -Michael >=20 >> On 17 Nov 2021, at 20:11, Peter M=C3=BCller w= rote: >> >> 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=C3=BCller >> >> Thanks, and best regards, >> Peter M=C3=BCller >> >>> 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 cha= r* cmd) { >>> char chroot_cmd[STRING_SIZE]; >>> @@ -149,14 +142,53 @@ int hw_mount(const char* source, const char* targe= t, 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* des= t, 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* de= st, 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* de= st, 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, i= nt 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) { >=20 --===============4073372293091193843==--