[PATCH v2 0/5] work around part table type misreporting by "parted"
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 v1: https://listman.redhat.com/archives/libguestfs/2021-November/msg00211.html In version 2, "list-filesystems" works with the bogus MBR partition table, as long as the FAT variant in use is FAT16 or FAT32. (Parted completely chokes on bogus MBR + FAT12, but that's arguably a rare corner case with libguestfs.) See also the patch-level Notes sections. Thanks, Laszlo Laszlo Ersek (5): daemon/mkfs: disable creation of fake MBR partition table with "mkfs.fat" daemon/9p: fix wrong pathname in error message daemon/parted: simplify print_partition_table() prototype daemon/parted: work around part table type misreporting by "parted" daemon/listfs: don't call "sgdisk -i" on bogus MBR partition table entry daemon/9p.c | 4 ++-- daemon/listfs.ml | 5 +++++ daemon/mkfs.c | 27 +++++++++++++++++++++++++++ daemon/parted.c | 17 ++++++----------- daemon/parted.ml | 13 ++++++++----- daemon/utils.ml | 44 ++++++++++++++++++++++++++++++++++++++++++++ daemon/utils.mli | 10 ++++++++++ 7 files changed, 102 insertions(+), 18 deletions(-) base-commit: 9fda9110e6a7afc1d418665f8f3538d87b5c8a5b -- 2.19.1.3.g30247aa5d201
Search the usage output of "mkfs.fat" for "--mbr[="; cache the result for further invocations. If the option is supported, pass "--mbr=n" to "mkfs.fat". This will prevent the creation of a bogus partition table whose first (and only) entry describes a partition that contains the partition table. (Such a bogus partition table breaks "parted", which is a tool used by libguestfs extensively, both internally and in public libguestfs APIs.) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Acked-by: Richard W.M. Jones <rjones@redhat.com> --- Notes: v2: - no change - pick up Rich's A-b v1: Testing (per <https://bugzilla.redhat.com/show_bug.cgi?id=1931821#c0>): $ virt-make-fs -vx --format=raw --size=200M --type=vfat --label=TEST \ website ~/test_vfat.img > [...] > creating vfat filesystem on /dev/sda ... > libguestfs: trace: mkfs "vfat" "/dev/sda" "label:TEST" > [...] > commandrvf: stdout=n stderr=y flags=0x0 > commandrvf: mkfs.fat > Usage: mkfs.fat [OPTIONS] TARGET [BLOCKS] > Create FAT filesystem in TARGET, which can be a block device or file. Use only > up to BLOCKS 1024 byte blocks if specified. With the -C option, file TARGET will be > created with a size of 1024 bytes times BLOCKS, which must be specified. > > Options: > -a Disable alignment of data structures > -A Toggle Atari variant of the filesystem > -b SECTOR Select SECTOR as location of the FAT32 backup boot sector > -c Check device for bad blocks before creating the filesystem > -C Create file TARGET then create filesystem in it > -D NUMBER Write BIOS drive number NUMBER to boot sector > -f COUNT Create COUNT file allocation tables > -F SIZE Select FAT size SIZE (12, 16 or 32) > -g GEOM Select disk geometry: heads/sectors_per_track > -h NUMBER Write hidden sectors NUMBER to boot sector > -i VOLID Set volume ID to VOLID (a 32 bit hexadecimal number) > -I Ignore and disable safety checks > -l FILENAME Read bad blocks list from FILENAME > -m FILENAME Replace default error message in boot block with contents of FILENAME > -M TYPE Set media type in boot sector to TYPE > --mbr[=y|n|a] Fill (fake) MBR table with one partition which spans whole disk > -n LABEL Set volume name to LABEL (up to 11 characters long) > --codepage=N use DOS codepage N to encode label (default: 850) > -r COUNT Make room for at least COUNT entries in the root directory > -R COUNT Set minimal number of reserved sectors to COUNT > -s COUNT Set number of sectors per cluster to COUNT > -S SIZE Select a sector size of SIZE (a power of two, at least 512) > -v Verbose execution > --variant=TYPE Select variant TYPE of filesystem (standard or Atari) > > --invariant Use constants for randomly generated or time based values > --offset=SECTOR Write the filesystem at a specific sector into the device file. > --help Show this help message and exit > [...] > commandrvf: stdout=n stderr=y flags=0x0 > commandrvf: mkfs -t vfat -I --mbr=n -n TEST /dev/sda $ guestfish -a ~/test_vfat.img > ><fs> run > ><fs> list-filesystems > /dev/sda: vfat daemon/mkfs.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/daemon/mkfs.c b/daemon/mkfs.c index ba90cef2111c..4cb216cabe8e 100644 --- a/daemon/mkfs.c +++ b/daemon/mkfs.c @@ -30,6 +30,14 @@ #define MAX_ARGS 64 +enum fat_mbr_option { + FMO_UNCHECKED, + FMO_DOESNT_EXIST, + FMO_EXISTS, +}; + +static enum fat_mbr_option fat_mbr_option = FMO_UNCHECKED; + /* Takes optional arguments, consult optargs_bitmask. */ int do_mkfs (const char *fstype, const char *device, int blocksize, @@ -101,6 +109,25 @@ do_mkfs (const char *fstype, const char *device, int blocksize, STREQ (fstype, "msdos")) ADD_ARG (argv, i, "-I"); + /* Prevent mkfs.fat from creating a bogus partition table (RHBZ#1931821). */ + if (STREQ (fstype, "fat") || STREQ (fstype, "vfat") || + STREQ (fstype, "msdos")) { + if (fat_mbr_option == FMO_UNCHECKED) { + CLEANUP_FREE char *usage_err = NULL; + + fat_mbr_option = FMO_DOESNT_EXIST; + /* Invoking either version 3 of version 4 of mkfs.fat without any options + * will make it (a) print a usage summary to stderr, (b) exit with status + * 1. + */ + r = commandr (NULL, &usage_err, "mkfs.fat", (char *)NULL); + if (r == 1 && strstr (usage_err, "--mbr[=") != NULL) + fat_mbr_option = FMO_EXISTS; + } + if (fat_mbr_option == FMO_EXISTS) + ADD_ARG (argv, i, "--mbr=n"); + } + /* Process blocksize parameter if set. */ if (optargs_bitmask & GUESTFS_MKFS_BLOCKSIZE_BITMASK) { if (blocksize <= 0 || !is_power_of_2 (blocksize)) { -- 2.19.1.3.g30247aa5d201
The directory that readdir() and closedir() work on is BUS_PATH ("/sys/bus/virtio/drivers/9pnet_virtio"), not "/sys/block". Fix the error messages that are sent when readdir() or closedir() fails. (The invalid "sys/block" pathname could be a leftover from when the directory reading logic was (perhaps) copied from "daemon/sync.c".) Fixes: 5f10c3350338bbca735a74db26f98da968957bd9 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new patch: unrelated fix for a wart I'd come across while working the BZ daemon/9p.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/9p.c b/daemon/9p.c index 743a96abd449..9a3a5cfdf274 100644 --- a/daemon/9p.c +++ b/daemon/9p.c @@ -106,14 +106,14 @@ do_list_9p (void) /* Check readdir didn't fail */ if (errno != 0) { - reply_with_perror ("readdir: /sys/block"); + reply_with_perror ("readdir: " BUS_PATH); closedir (dir); return NULL; } /* Close the directory handle */ if (closedir (dir) == -1) { - reply_with_perror ("closedir: /sys/block"); + reply_with_perror ("closedir: " BUS_PATH); return NULL; } -- 2.19.1.3.g30247aa5d201
Since commit 994ca1f8ebcc ("daemon: Reimplement 'part_get_mbr_part_type' API in OCaml.", 2018-05-02), we've not had any calls to print_partition_table() that would pass a "true" argument for the "add_m_option" parameter. Remove the parameter, and inside part_get_mbr_part_type(), remove the dead branch. Relatedly, update the comment on the "print_partition_table_machine_readable" OCaml function, originally from commit 32e661f42169 ("daemon: Reimplement ‘part_list’ API in OCaml.", 2017-07-27). Because print_partition_table() now passes "-m" to "parted" unconditionally, and there are no use cases left that would *forbid* "-m", "print_partition_table_machine_readable" is almost equivalent to print_partition_table() -- modulo the enforcement of the "BYT;" header. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new patch: unrelated refactoring I'd found possible while working the BZ daemon/parted.c | 17 ++++++----------- daemon/parted.ml | 6 ++---- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/daemon/parted.c b/daemon/parted.c index ef7b90056b41..3a723762c345 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -341,7 +341,7 @@ get_table_field (const char *line, int n) } static char * -print_partition_table (const char *device, bool add_m_option) +print_partition_table (const char *device) { char *out; CLEANUP_FREE char *err = NULL; @@ -349,14 +349,9 @@ print_partition_table (const char *device, bool add_m_option) udev_settle (); - if (add_m_option) - r = command (&out, &err, "parted", "-m", "-s", "--", device, - "unit", "b", - "print", NULL); - else - r = command (&out, &err, "parted", "-s", "--", device, - "unit", "b", - "print", NULL); + r = command (&out, &err, "parted", "-m", "-s", "--", device, + "unit", "b", + "print", NULL); udev_settle (); @@ -383,7 +378,7 @@ do_part_get_bootable (const char *device, int partnum) return -1; } - CLEANUP_FREE char *out = print_partition_table (device, true); + CLEANUP_FREE char *out = print_partition_table (device); if (!out) return -1; @@ -555,7 +550,7 @@ do_part_get_name (const char *device, int partnum) return NULL; if (STREQ (parttype, "gpt")) { - CLEANUP_FREE char *out = print_partition_table (device, true); + CLEANUP_FREE char *out = print_partition_table (device); if (!out) return NULL; diff --git a/daemon/parted.ml b/daemon/parted.ml index 45c387987483..29b98c4806c1 100644 --- a/daemon/parted.ml +++ b/daemon/parted.ml @@ -57,10 +57,8 @@ let part_get_mbr_id device partnum = (* It's printed in hex, possibly with a leading space. *) sscanf out " %x" identity -(* This is not equivalent to print_partition_table in the C code, as - * it only deals with the ‘-m’ option output, and it partially parses - * that. If we convert other functions that don't use the ‘-m’ version - * we'll have to refactor this. XXX +(* This is almost equivalent to print_partition_table in the C code. The + * difference is that here we enforce the "BYT;" header internally. *) let print_partition_table_machine_readable device = udev_settle (); -- 2.19.1.3.g30247aa5d201
On 11/24/21 11:37, Laszlo Ersek wrote:
Since commit 994ca1f8ebcc ("daemon: Reimplement 'part_get_mbr_part_type' API in OCaml.", 2018-05-02), we've not had any calls to print_partition_table() that would pass a "true" argument for the
Typo: should be '"false" argument'. Will fix upon merge or in v3. Sorry! Laszlo
"add_m_option" parameter.
Remove the parameter, and inside part_get_mbr_part_type(), remove the dead branch.
Relatedly, update the comment on the "print_partition_table_machine_readable" OCaml function, originally from commit 32e661f42169 ("daemon: Reimplement ‘part_list’ API in OCaml.", 2017-07-27). Because print_partition_table() now passes "-m" to "parted" unconditionally, and there are no use cases left that would *forbid* "-m", "print_partition_table_machine_readable" is almost equivalent to print_partition_table() -- modulo the enforcement of the "BYT;" header.
Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: v2:
- new patch: unrelated refactoring I'd found possible while working the BZ
daemon/parted.c | 17 ++++++----------- daemon/parted.ml | 6 ++---- 2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/daemon/parted.c b/daemon/parted.c index ef7b90056b41..3a723762c345 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -341,7 +341,7 @@ get_table_field (const char *line, int n) }
static char * -print_partition_table (const char *device, bool add_m_option) +print_partition_table (const char *device) { char *out; CLEANUP_FREE char *err = NULL; @@ -349,14 +349,9 @@ print_partition_table (const char *device, bool add_m_option)
udev_settle ();
- if (add_m_option) - r = command (&out, &err, "parted", "-m", "-s", "--", device, - "unit", "b", - "print", NULL); - else - r = command (&out, &err, "parted", "-s", "--", device, - "unit", "b", - "print", NULL); + r = command (&out, &err, "parted", "-m", "-s", "--", device, + "unit", "b", + "print", NULL);
udev_settle ();
@@ -383,7 +378,7 @@ do_part_get_bootable (const char *device, int partnum) return -1; }
- CLEANUP_FREE char *out = print_partition_table (device, true); + CLEANUP_FREE char *out = print_partition_table (device); if (!out) return -1;
@@ -555,7 +550,7 @@ do_part_get_name (const char *device, int partnum) return NULL;
if (STREQ (parttype, "gpt")) { - CLEANUP_FREE char *out = print_partition_table (device, true); + CLEANUP_FREE char *out = print_partition_table (device); if (!out) return NULL;
diff --git a/daemon/parted.ml b/daemon/parted.ml index 45c387987483..29b98c4806c1 100644 --- a/daemon/parted.ml +++ b/daemon/parted.ml @@ -57,10 +57,8 @@ let part_get_mbr_id device partnum = (* It's printed in hex, possibly with a leading space. *) sscanf out " %x" identity
-(* This is not equivalent to print_partition_table in the C code, as - * it only deals with the ‘-m’ option output, and it partially parses - * that. If we convert other functions that don't use the ‘-m’ version - * we'll have to refactor this. XXX +(* This is almost equivalent to print_partition_table in the C code. The + * difference is that here we enforce the "BYT;" header internally. *) let print_partition_table_machine_readable device = udev_settle ();
On Wed, Nov 24, 2021 at 11:43:08AM +0100, Laszlo Ersek wrote:
On 11/24/21 11:37, Laszlo Ersek wrote:
Since commit 994ca1f8ebcc ("daemon: Reimplement 'part_get_mbr_part_type' API in OCaml.", 2018-05-02), we've not had any calls to print_partition_table() that would pass a "true" argument for the
Typo: should be '"false" argument'.
Will fix upon merge or in v3.
Patches 1-3 are good, ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
"parted" incorrectly reports "loop" rather than "msdos" for the partition table type, when the (fake) partition table comes from the "--mbr" option of "mkfs.fat" (in dosfstools-4.2+), and the FAT variant in question is FAT16 or FAT32. (See RHBZ#2026224.) Work this around by - parsing the partition table ourselves, and - overriding "loop" with "msdos" when appropriate. Note that when the FAT variant is FAT12, "parted" fails to parse the fake MBR partition table completely (see RHBZ#2026220), which we cannot work around. However, FAT12 should be a rare corner case in libguestfs usage -- "mkfs.fat" auto-chooses FAT12 only below 9MB disk size, and even "-F 12" can only be forced up to and including 255MB disk size. Add the helper function "has_bogus_mbr" to the Utils module; we'll use it elsewhere too. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new patch (the hard one) daemon/parted.ml | 7 ++++++- daemon/utils.ml | 44 ++++++++++++++++++++++++++++++++++++++++++++ daemon/utils.mli | 10 ++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/daemon/parted.ml b/daemon/parted.ml index 29b98c4806c1..53077d91afdd 100644 --- a/daemon/parted.ml +++ b/daemon/parted.ml @@ -118,7 +118,12 @@ let part_get_parttype device = let fields = String.nsplit ":" device_line in match fields with | _::_::_::_::_::"loop"::_ -> (* If "loop" return an error (RHBZ#634246). *) - failwithf "%s: not a partitioned device" device + (* ... Unless parted failed to recognize the fake MBR that mkfs.fat from + * dosfstools-4.2+ created. In that case, return "msdos" for MBR + * (RHBZ#1931821). + *) + if Utils.has_bogus_mbr device then "msdos" + else failwithf "%s: not a partitioned device" device | _::_::_::_::_::ret::_ -> ret | _ -> failwithf "%s: cannot parse the output of parted" device diff --git a/daemon/utils.ml b/daemon/utils.ml index b56306055634..7ef3c206b71a 100644 --- a/daemon/utils.ml +++ b/daemon/utils.ml @@ -187,6 +187,50 @@ and compare_device_names a b = ) ) +let has_bogus_mbr device = + try + with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd -> + let sec0size = 0x200 + and sigofs = 0x1FE + and sysidofs = 0x003 and sysidsize = 0x008 + and pte1ofs = 0x1BE + and parttypes = [0x01; (* FAT12 *) + 0x04; (* FAT16 *) + 0x06; (* FAT12, FAT16, FAT16B *) + 0x0C; (* FAT32 LBA *) + 0x0E (* FAT16B LBA *)] in + let sec0 = Bytes.create sec0size in + let sec0read = Unix.read fd sec0 0 sec0size in + + (* sector read completely *) + sec0read = sec0size && + + (* boot signature present *) + Bytes.get_uint8 sec0 (sigofs ) = 0x55 && + Bytes.get_uint8 sec0 (sigofs + 0x1) = 0xAA && + + (* mkfs.fat signature present *) + Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat" && + + (* partition bootable *) + Bytes.get_uint8 sec0 (pte1ofs ) = 0x80 && + + (* partition starts at C/H/S 0/0/1 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x1) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x2) = 0x01 && + Bytes.get_uint8 sec0 (pte1ofs + 0x3) = 0x00 && + + (* partition type is a FAT variant that mkfs.fat is known to create *) + List.mem (Bytes.get_uint8 sec0 (pte1ofs + 0x4)) parttypes && + + (* partition starts at LBA 0 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x8) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x9) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xA) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xB) = 0x00 + ) + with _ -> false + let proc_unmangle_path path = let n = String.length path in let b = Buffer.create n in diff --git a/daemon/utils.mli b/daemon/utils.mli index ec1127f7211e..d7b7aeba0657 100644 --- a/daemon/utils.mli +++ b/daemon/utils.mli @@ -56,6 +56,16 @@ val sort_device_names : string list -> string list This also deals with partition numbers, and works whether or not [/dev/] is present. *) +val has_bogus_mbr : string -> bool +(** Check whether the first sector of the device contains a bogus MBR partition + table; namely one where the first partition table entry describes a + partition that starts at absolute sector 0, thereby overlapping the + partition table itself. + + dosfstools-4.2+ creates bogus partition tables like this by default when + formatting non-removable, non-partitioned block devices. Refer to + RHBZ#1931821. *) + val proc_unmangle_path : string -> string (** Reverse kernel path escaping done in fs/seq_file.c:mangle_path. This is inconsistently used for /proc fields. *) -- 2.19.1.3.g30247aa5d201
On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek wrote:
"parted" incorrectly reports "loop" rather than "msdos" for the partition table type, when the (fake) partition table comes from the "--mbr" option of "mkfs.fat" (in dosfstools-4.2+), and the FAT variant in question is FAT16 or FAT32. (See RHBZ#2026224.) Work this around by - parsing the partition table ourselves, and - overriding "loop" with "msdos" when appropriate.
Note that when the FAT variant is FAT12, "parted" fails to parse the fake MBR partition table completely (see RHBZ#2026220), which we cannot work around. However, FAT12 should be a rare corner case in libguestfs usage -- "mkfs.fat" auto-chooses FAT12 only below 9MB disk size, and even "-F 12" can only be forced up to and including 255MB disk size.
Add the helper function "has_bogus_mbr" to the Utils module; we'll use it elsewhere too.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: v2:
- new patch (the hard one)
daemon/parted.ml | 7 ++++++- daemon/utils.ml | 44 ++++++++++++++++++++++++++++++++++++++++++++ daemon/utils.mli | 10 ++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/daemon/parted.ml b/daemon/parted.ml index 29b98c4806c1..53077d91afdd 100644 --- a/daemon/parted.ml +++ b/daemon/parted.ml @@ -118,7 +118,12 @@ let part_get_parttype device = let fields = String.nsplit ":" device_line in match fields with | _::_::_::_::_::"loop"::_ -> (* If "loop" return an error (RHBZ#634246). *) - failwithf "%s: not a partitioned device" device + (* ... Unless parted failed to recognize the fake MBR that mkfs.fat from + * dosfstools-4.2+ created. In that case, return "msdos" for MBR + * (RHBZ#1931821). + *) + if Utils.has_bogus_mbr device then "msdos" + else failwithf "%s: not a partitioned device" device | _::_::_::_::_::ret::_ -> ret | _ -> failwithf "%s: cannot parse the output of parted" device diff --git a/daemon/utils.ml b/daemon/utils.ml index b56306055634..7ef3c206b71a 100644 --- a/daemon/utils.ml +++ b/daemon/utils.ml @@ -187,6 +187,50 @@ and compare_device_names a b = ) )
+let has_bogus_mbr device = + try + with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd -> + let sec0size = 0x200 + and sigofs = 0x1FE + and sysidofs = 0x003 and sysidsize = 0x008 + and pte1ofs = 0x1BE + and parttypes = [0x01; (* FAT12 *) + 0x04; (* FAT16 *) + 0x06; (* FAT12, FAT16, FAT16B *) + 0x0C; (* FAT32 LBA *) + 0x0E (* FAT16B LBA *)] in + let sec0 = Bytes.create sec0size in + let sec0read = Unix.read fd sec0 0 sec0size in + + (* sector read completely *) + sec0read = sec0size && + + (* boot signature present *) + Bytes.get_uint8 sec0 (sigofs ) = 0x55 && + Bytes.get_uint8 sec0 (sigofs + 0x1) = 0xAA && + + (* mkfs.fat signature present *) + Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat" && + + (* partition bootable *) + Bytes.get_uint8 sec0 (pte1ofs ) = 0x80 && + + (* partition starts at C/H/S 0/0/1 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x1) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x2) = 0x01 && + Bytes.get_uint8 sec0 (pte1ofs + 0x3) = 0x00 && + + (* partition type is a FAT variant that mkfs.fat is known to create *) + List.mem (Bytes.get_uint8 sec0 (pte1ofs + 0x4)) parttypes && + + (* partition starts at LBA 0 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x8) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x9) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xA) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xB) = 0x00 + ) + with _ -> false
It's not wrong, but you might consider factoring out some of those common functions. First of all, take a look at this code for inspiration since it's doing something which is a bit similar: https://github.com/libguestfs/libguestfs/blob/e7f72ab146b9c2aaee92a600a1fcbe... Using factoring you could write it as: let is offs b = Bytes.get_uint8 sec0 offs = b in is sigofs 0x55 && is (sigofs + 0x1) 0xaa && ... It's a matter of taste though, not a blocker. Looks good, ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 11/24/21 17:45, Richard W.M. Jones wrote:
On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek wrote:
"parted" incorrectly reports "loop" rather than "msdos" for the partition table type, when the (fake) partition table comes from the "--mbr" option of "mkfs.fat" (in dosfstools-4.2+), and the FAT variant in question is FAT16 or FAT32. (See RHBZ#2026224.) Work this around by - parsing the partition table ourselves, and - overriding "loop" with "msdos" when appropriate.
Note that when the FAT variant is FAT12, "parted" fails to parse the fake MBR partition table completely (see RHBZ#2026220), which we cannot work around. However, FAT12 should be a rare corner case in libguestfs usage -- "mkfs.fat" auto-chooses FAT12 only below 9MB disk size, and even "-F 12" can only be forced up to and including 255MB disk size.
Add the helper function "has_bogus_mbr" to the Utils module; we'll use it elsewhere too.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: v2:
- new patch (the hard one)
daemon/parted.ml | 7 ++++++- daemon/utils.ml | 44 ++++++++++++++++++++++++++++++++++++++++++++ daemon/utils.mli | 10 ++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/daemon/parted.ml b/daemon/parted.ml index 29b98c4806c1..53077d91afdd 100644 --- a/daemon/parted.ml +++ b/daemon/parted.ml @@ -118,7 +118,12 @@ let part_get_parttype device = let fields = String.nsplit ":" device_line in match fields with | _::_::_::_::_::"loop"::_ -> (* If "loop" return an error (RHBZ#634246). *) - failwithf "%s: not a partitioned device" device + (* ... Unless parted failed to recognize the fake MBR that mkfs.fat from + * dosfstools-4.2+ created. In that case, return "msdos" for MBR + * (RHBZ#1931821). + *) + if Utils.has_bogus_mbr device then "msdos" + else failwithf "%s: not a partitioned device" device | _::_::_::_::_::ret::_ -> ret | _ -> failwithf "%s: cannot parse the output of parted" device diff --git a/daemon/utils.ml b/daemon/utils.ml index b56306055634..7ef3c206b71a 100644 --- a/daemon/utils.ml +++ b/daemon/utils.ml @@ -187,6 +187,50 @@ and compare_device_names a b = ) )
+let has_bogus_mbr device = + try + with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd -> + let sec0size = 0x200 + and sigofs = 0x1FE + and sysidofs = 0x003 and sysidsize = 0x008 + and pte1ofs = 0x1BE + and parttypes = [0x01; (* FAT12 *) + 0x04; (* FAT16 *) + 0x06; (* FAT12, FAT16, FAT16B *) + 0x0C; (* FAT32 LBA *) + 0x0E (* FAT16B LBA *)] in + let sec0 = Bytes.create sec0size in + let sec0read = Unix.read fd sec0 0 sec0size in + + (* sector read completely *) + sec0read = sec0size && + + (* boot signature present *) + Bytes.get_uint8 sec0 (sigofs ) = 0x55 && + Bytes.get_uint8 sec0 (sigofs + 0x1) = 0xAA && + + (* mkfs.fat signature present *) + Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat" && + + (* partition bootable *) + Bytes.get_uint8 sec0 (pte1ofs ) = 0x80 && + + (* partition starts at C/H/S 0/0/1 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x1) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x2) = 0x01 && + Bytes.get_uint8 sec0 (pte1ofs + 0x3) = 0x00 && + + (* partition type is a FAT variant that mkfs.fat is known to create *) + List.mem (Bytes.get_uint8 sec0 (pte1ofs + 0x4)) parttypes && + + (* partition starts at LBA 0 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x8) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x9) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xA) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xB) = 0x00 + ) + with _ -> false
It's not wrong, but you might consider factoring out some of those common functions.
First of all, take a look at this code for inspiration since it's doing something which is a bit similar: https://github.com/libguestfs/libguestfs/blob/e7f72ab146b9c2aaee92a600a1fcbe...
- Upon reading the documentation of the Bytes module <https://ocaml.org/api/Bytes.html>, I've found the to_string function. That just rubbed me the wrong way, completely. I don't want to deal with characters here (except for the "system id", where I do use sub_string), but with uint8_t values. IOW I wouldn't like to think about characters here, and make comparisons against character constants. Anyway that's just a generic comment. - What does the "|>" operator do?
Using factoring you could write it as:
let is offs b = Bytes.get_uint8 sec0 offs = b in is sigofs 0x55 && is (sigofs + 0x1) 0xaa && ...
I was slightly annoyed by having to write down "Bytes.get_uint8 sec0" so many times; on the other hand, I feel "attached" to the equal sign (perhaps because it resembles C more?). My preferred form would be something that resembles array indexing (but that was when I found out about "to_string", and I refused to go through that, even temporarily, for conversion to a byte array perhaps). Maybe I could do let sec0_at = fun offset -> Bytes.get_uint8 sec0 offset and then write sec0_at (sigofs ) = 0x55 && sec0_at (sigofs + 0x1) = 0xAA && (... It's less concise than your "is", but with "maximal factoring", every such block reads like its own domain-specific language.) Does "sec0_at" feel like an improvement to you? If it does, I'll respin.
It's a matter of taste though, not a blocker.
Well I do want to learn good taste in OCaml. :)
Looks good, ACK.
Thanks! Laszlo
On Thu, Nov 25, 2021 at 09:40:29AM +0100, Laszlo Ersek wrote:
On 11/24/21 17:45, Richard W.M. Jones wrote:
On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek wrote:
"parted" incorrectly reports "loop" rather than "msdos" for the partition table type, when the (fake) partition table comes from the "--mbr" option of "mkfs.fat" (in dosfstools-4.2+), and the FAT variant in question is FAT16 or FAT32. (See RHBZ#2026224.) Work this around by - parsing the partition table ourselves, and - overriding "loop" with "msdos" when appropriate.
Note that when the FAT variant is FAT12, "parted" fails to parse the fake MBR partition table completely (see RHBZ#2026220), which we cannot work around. However, FAT12 should be a rare corner case in libguestfs usage -- "mkfs.fat" auto-chooses FAT12 only below 9MB disk size, and even "-F 12" can only be forced up to and including 255MB disk size.
Add the helper function "has_bogus_mbr" to the Utils module; we'll use it elsewhere too.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: v2:
- new patch (the hard one)
daemon/parted.ml | 7 ++++++- daemon/utils.ml | 44 ++++++++++++++++++++++++++++++++++++++++++++ daemon/utils.mli | 10 ++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/daemon/parted.ml b/daemon/parted.ml index 29b98c4806c1..53077d91afdd 100644 --- a/daemon/parted.ml +++ b/daemon/parted.ml @@ -118,7 +118,12 @@ let part_get_parttype device = let fields = String.nsplit ":" device_line in match fields with | _::_::_::_::_::"loop"::_ -> (* If "loop" return an error (RHBZ#634246). *) - failwithf "%s: not a partitioned device" device + (* ... Unless parted failed to recognize the fake MBR that mkfs.fat from + * dosfstools-4.2+ created. In that case, return "msdos" for MBR + * (RHBZ#1931821). + *) + if Utils.has_bogus_mbr device then "msdos" + else failwithf "%s: not a partitioned device" device | _::_::_::_::_::ret::_ -> ret | _ -> failwithf "%s: cannot parse the output of parted" device diff --git a/daemon/utils.ml b/daemon/utils.ml index b56306055634..7ef3c206b71a 100644 --- a/daemon/utils.ml +++ b/daemon/utils.ml @@ -187,6 +187,50 @@ and compare_device_names a b = ) )
+let has_bogus_mbr device = + try + with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd -> + let sec0size = 0x200 + and sigofs = 0x1FE + and sysidofs = 0x003 and sysidsize = 0x008 + and pte1ofs = 0x1BE + and parttypes = [0x01; (* FAT12 *) + 0x04; (* FAT16 *) + 0x06; (* FAT12, FAT16, FAT16B *) + 0x0C; (* FAT32 LBA *) + 0x0E (* FAT16B LBA *)] in + let sec0 = Bytes.create sec0size in + let sec0read = Unix.read fd sec0 0 sec0size in + + (* sector read completely *) + sec0read = sec0size && + + (* boot signature present *) + Bytes.get_uint8 sec0 (sigofs ) = 0x55 && + Bytes.get_uint8 sec0 (sigofs + 0x1) = 0xAA && + + (* mkfs.fat signature present *) + Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat" && + + (* partition bootable *) + Bytes.get_uint8 sec0 (pte1ofs ) = 0x80 && + + (* partition starts at C/H/S 0/0/1 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x1) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x2) = 0x01 && + Bytes.get_uint8 sec0 (pte1ofs + 0x3) = 0x00 && + + (* partition type is a FAT variant that mkfs.fat is known to create *) + List.mem (Bytes.get_uint8 sec0 (pte1ofs + 0x4)) parttypes && + + (* partition starts at LBA 0 *) + Bytes.get_uint8 sec0 (pte1ofs + 0x8) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0x9) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xA) = 0x00 && + Bytes.get_uint8 sec0 (pte1ofs + 0xB) = 0x00 + ) + with _ -> false
It's not wrong, but you might consider factoring out some of those common functions.
First of all, take a look at this code for inspiration since it's doing something which is a bit similar: https://github.com/libguestfs/libguestfs/blob/e7f72ab146b9c2aaee92a600a1fcbe...
- Upon reading the documentation of the Bytes module <https://ocaml.org/api/Bytes.html>, I've found the to_string function.
That just rubbed me the wrong way, completely. I don't want to deal with characters here (except for the "system id", where I do use sub_string), but with uint8_t values. IOW I wouldn't like to think about characters here, and make comparisons against character constants. Anyway that's just a generic comment.
char == octet in OCaml, there's no difference. If you want Unicode codepoints then you have to use the Camomile library (https://github.com/yoriyuki/Camomile/) but we don't even have that in RHEL. Actually I prefer it this way over languages like Python where everything is interpreted as utf-8 and you have to go through hoops to get a bag of bytes.
- What does the "|>" operator do?
It's a pipeline of operations: f arg <==> arg |> f g (f arg) <==> arg |> f |> g eg: # 123 |> string_of_int |> String.length ;; - : int = 3
Using factoring you could write it as:
let is offs b = Bytes.get_uint8 sec0 offs = b in is sigofs 0x55 && is (sigofs + 0x1) 0xaa && ...
I was slightly annoyed by having to write down "Bytes.get_uint8 sec0" so many times; on the other hand, I feel "attached" to the equal sign (perhaps because it resembles C more?). My preferred form would be something that resembles array indexing (but that was when I found out about "to_string", and I refused to go through that, even temporarily, for conversion to a byte array perhaps).
Maybe I could do
let sec0_at = fun offset -> Bytes.get_uint8 sec0 offset
The use of fun is very awkward here. The natural way to write this (with some currying) is: let sec0_at = Bytes.get_uint8 sec0
and then write
sec0_at (sigofs ) = 0x55 && sec0_at (sigofs + 0x1) = 0xAA &&
(... It's less concise than your "is", but with "maximal factoring", every such block reads like its own domain-specific language.)
Does "sec0_at" feel like an improvement to you? If it does, I'll respin.
Yes, that looks better than my version actually. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek wrote:
+let has_bogus_mbr device = + try + with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd -> + let sec0size = 0x200 + and sigofs = 0x1FE + and sysidofs = 0x003 and sysidsize = 0x008 + and pte1ofs = 0x1BE + and parttypes = [0x01; (* FAT12 *) + 0x04; (* FAT16 *) + 0x06; (* FAT12, FAT16, FAT16B *) + 0x0C; (* FAT32 LBA *) + 0x0E (* FAT16B LBA *)] in + let sec0 = Bytes.create sec0size in + let sec0read = Unix.read fd sec0 0 sec0size in
I forgot to say that since Unix has been "open"-d here, you don't need the "Unix." prefix, unless you think it's necessary for clarity about which particular read function is being called. (I personally wouldn't use it because it's like the read(2) function in C which also doesn't have a prefix.) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On 11/24/21 17:47, Richard W.M. Jones wrote:
On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek wrote:
+let has_bogus_mbr device = + try + with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd -> + let sec0size = 0x200 + and sigofs = 0x1FE + and sysidofs = 0x003 and sysidsize = 0x008 + and pte1ofs = 0x1BE + and parttypes = [0x01; (* FAT12 *) + 0x04; (* FAT16 *) + 0x06; (* FAT12, FAT16, FAT16B *) + 0x0C; (* FAT32 LBA *) + 0x0E (* FAT16B LBA *)] in + let sec0 = Bytes.create sec0size in + let sec0read = Unix.read fd sec0 0 sec0size in
I forgot to say that since Unix has been "open"-d here, you don't need the "Unix." prefix, unless you think it's necessary for clarity about which particular read function is being called.
FWIW it does make the code easier for me to understand.
(I personally wouldn't use it because it's like the read(2) function in C which also doesn't have a prefix.)
C does not have namespaces (in the sense that OCaml and C++ do), so there is only one read() in it (well, in POSIX). I don't know enough OCaml to tell when a bare "read" implies "Unix.read" to the (seasoned) reader. :) Interestingly, while the argument does not seem to work for me with "read", it certainly does with something like "printf"! ... Hm, let me try to apply the same idea to the many "Bytes.get_uint8" function calls... If I opened Bytes at the top, I could write get_uint8 sec0 (sigofs ) = 0x55 && get_uint8 sec0 (sigofs + 0x1) = 0xAA && (* mkfs.fat signature present *) sub_string sec0 sysidofs sysidsize = "mkfs.fat" && (* partition bootable *) get_uint8 sec0 (pte1ofs ) = 0x80 && (* partition starts at C/H/S 0/0/1 *) get_uint8 sec0 (pte1ofs + 0x1) = 0x00 && get_uint8 sec0 (pte1ofs + 0x2) = 0x01 && get_uint8 sec0 (pte1ofs + 0x3) = 0x00 && (* partition type is a FAT variant that mkfs.fat is known to create *) List.mem (get_uint8 sec0 (pte1ofs + 0x4)) parttypes && (* partition starts at LBA 0 *) get_uint8 sec0 (pte1ofs + 0x8) = 0x00 && get_uint8 sec0 (pte1ofs + 0x9) = 0x00 && get_uint8 sec0 (pte1ofs + 0xA) = 0x00 && get_uint8 sec0 (pte1ofs + 0xB) = 0x00 No, I don't like that. I wouldn't know where (in which module) to look up the documentation of "get_uint8" and "sub_string". I'd have to consult the open's at the top, and maybe look up "get_uint8" in each? When I call a function, I'd like it to be uniquely identified on the spot, with as little context as possible. I think. (In fact, I might not be able to replace Bytes.create with just "create", as (IIRC) "create" is a heavily reused function name in OCaml.) But... I don't want to add non-idiomatic code. I'll respin and drop the "Unix." qualifier from "read". Hopefully I'll get used to it! Thanks! Laszlo
On Thu, Nov 25, 2021 at 09:54:30AM +0100, Laszlo Ersek wrote:
On 11/24/21 17:47, Richard W.M. Jones wrote:
On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek wrote:
+let has_bogus_mbr device = + try + with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd -> + let sec0size = 0x200 + and sigofs = 0x1FE + and sysidofs = 0x003 and sysidsize = 0x008 + and pte1ofs = 0x1BE + and parttypes = [0x01; (* FAT12 *) + 0x04; (* FAT16 *) + 0x06; (* FAT12, FAT16, FAT16B *) + 0x0C; (* FAT32 LBA *) + 0x0E (* FAT16B LBA *)] in + let sec0 = Bytes.create sec0size in + let sec0read = Unix.read fd sec0 0 sec0size in
I forgot to say that since Unix has been "open"-d here, you don't need the "Unix." prefix, unless you think it's necessary for clarity about which particular read function is being called.
FWIW it does make the code easier for me to understand.
(I personally wouldn't use it because it's like the read(2) function in C which also doesn't have a prefix.)
C does not have namespaces (in the sense that OCaml and C++ do), so there is only one read() in it (well, in POSIX).
I don't know enough OCaml to tell when a bare "read" implies "Unix.read" to the (seasoned) reader. :)
Interestingly, while the argument does not seem to work for me with "read", it certainly does with something like "printf"!
... Hm, let me try to apply the same idea to the many "Bytes.get_uint8" function calls... If I opened Bytes at the top, I could write
get_uint8 sec0 (sigofs ) = 0x55 && get_uint8 sec0 (sigofs + 0x1) = 0xAA &&
I'd be cautious about open-ing too many modules. In particular it's usually not a good idea to open String, Bytes, List, Array, etc. because they contain type definitions and functions with conflicting names. Worse still, if the version of OCaml is upgraded this can cause future problems [see below]. The implied rule here is that some modules are meant to be opened (Unix, Printf, some of the modules we've written) and most are not. Here's an example of a real problem caused by someone using "open List": https://bugzilla.redhat.com/show_bug.cgi?id=1987488#c5 https://src.fedoraproject.org/rpms/freetennis/c/24fc04eb97d718b4d8514a1ac075...
(* mkfs.fat signature present *) sub_string sec0 sysidofs sysidsize = "mkfs.fat" &&
(* partition bootable *) get_uint8 sec0 (pte1ofs ) = 0x80 &&
(* partition starts at C/H/S 0/0/1 *) get_uint8 sec0 (pte1ofs + 0x1) = 0x00 && get_uint8 sec0 (pte1ofs + 0x2) = 0x01 && get_uint8 sec0 (pte1ofs + 0x3) = 0x00 &&
(* partition type is a FAT variant that mkfs.fat is known to create *) List.mem (get_uint8 sec0 (pte1ofs + 0x4)) parttypes &&
(* partition starts at LBA 0 *) get_uint8 sec0 (pte1ofs + 0x8) = 0x00 && get_uint8 sec0 (pte1ofs + 0x9) = 0x00 && get_uint8 sec0 (pte1ofs + 0xA) = 0x00 && get_uint8 sec0 (pte1ofs + 0xB) = 0x00
No, I don't like that. I wouldn't know where (in which module) to look up the documentation of "get_uint8" and "sub_string". I'd have to consult the open's at the top, and maybe look up "get_uint8" in each? When I call a function, I'd like it to be uniquely identified on the spot, with as little context as possible. I think.
(In fact, I might not be able to replace Bytes.create with just "create", as (IIRC) "create" is a heavily reused function name in OCaml.)
But... I don't want to add non-idiomatic code. I'll respin and drop the "Unix." qualifier from "read". Hopefully I'll get used to it!
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 11/25/21 11:10, Richard W.M. Jones wrote:
The implied rule here is that some modules are meant to be opened (Unix, Printf, some of the modules we've written) and most are not.
Here's an example of a real problem caused by someone using "open List":
https://bugzilla.redhat.com/show_bug.cgi?id=1987488#c5 https://src.fedoraproject.org/rpms/freetennis/c/24fc04eb97d718b4d8514a1ac075...
Thanks! This is the kind of "lore" that I was looking for :) Laszlo
The "is_partition_can_hold_filesystem" function calls "Parted.part_get_gpt_type" on the partition if: - the partition table type is GPT, - or the partition table type is MBR, and the partition is primary or logical. The one entry in the fake MBR partition table described in the previous patch passes the second branch of this check, therefore "Parted.part_get_gpt_type" is reached, and it invokes "sgdisk -i 1" on the disk. Surprisingly (not), while "sgdisk -i" copes fine with valid MBR partition tables, it chokes on the fake one. The output does not contain the "Partition GUID code" line, and so "sgdisk_info_extract_field" throws an exception. Prevent calling "Parted.part_get_gpt_type" on a bogus MBR partition table, similarly to the "extended entry in MBR partition table" case; the difference is that the bogus primary entry, unlike a valid extended entry, *can* hold a filesystem. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new patch (sigh) daemon/listfs.ml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/daemon/listfs.ml b/daemon/listfs.ml index d8add5005ade..63ced260a14d 100644 --- a/daemon/listfs.ml +++ b/daemon/listfs.ml @@ -109,6 +109,8 @@ and is_partition_can_hold_filesystem partition = if is_gpt_or_mbr then ( if is_mbr_extended parttype device partnum then false + else if is_mbr_bogus parttype device partnum then + true else ( (* MBR partition id will be converted into corresponding GPT type. *) let gpt_type = Parted.part_get_gpt_type device partnum in @@ -130,6 +132,9 @@ and is_mbr_extended parttype device partnum = parttype = "msdos" && Parted.part_get_mbr_part_type device partnum = "extended" +and is_mbr_bogus parttype device partnum = + parttype = "msdos" && partnum = 1 && Utils.has_bogus_mbr device + (* Use vfs-type to check for a filesystem of some sort of [device]. * Appends (device, vfs_type) to the ret parameter (there may be * multiple devices found in the case of btrfs). -- 2.19.1.3.g30247aa5d201
On Wed, Nov 24, 2021 at 11:37:47AM +0100, Laszlo Ersek wrote:
The "is_partition_can_hold_filesystem" function calls "Parted.part_get_gpt_type" on the partition if: - the partition table type is GPT, - or the partition table type is MBR, and the partition is primary or logical.
The one entry in the fake MBR partition table described in the previous patch passes the second branch of this check, therefore "Parted.part_get_gpt_type" is reached, and it invokes "sgdisk -i 1" on the disk.
Surprisingly (not), while "sgdisk -i" copes fine with valid MBR partition tables, it chokes on the fake one. The output does not contain the "Partition GUID code" line, and so "sgdisk_info_extract_field" throws an exception.
Prevent calling "Parted.part_get_gpt_type" on a bogus MBR partition table, similarly to the "extended entry in MBR partition table" case; the difference is that the bogus primary entry, unlike a valid extended entry, *can* hold a filesystem.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: v2:
- new patch (sigh)
daemon/listfs.ml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/daemon/listfs.ml b/daemon/listfs.ml index d8add5005ade..63ced260a14d 100644 --- a/daemon/listfs.ml +++ b/daemon/listfs.ml @@ -109,6 +109,8 @@ and is_partition_can_hold_filesystem partition = if is_gpt_or_mbr then ( if is_mbr_extended parttype device partnum then false + else if is_mbr_bogus parttype device partnum then + true else ( (* MBR partition id will be converted into corresponding GPT type. *) let gpt_type = Parted.part_get_gpt_type device partnum in @@ -130,6 +132,9 @@ and is_mbr_extended parttype device partnum = parttype = "msdos" && Parted.part_get_mbr_part_type device partnum = "extended"
+and is_mbr_bogus parttype device partnum = + parttype = "msdos" && partnum = 1 && Utils.has_bogus_mbr device + (* Use vfs-type to check for a filesystem of some sort of [device]. * Appends (device, vfs_type) to the ret parameter (there may be * multiple devices found in the case of btrfs).
Looks reasonable, ACK. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones