Laszlo Ersek
2021-Nov-25 15:27 UTC
[Libguestfs] [PATCH v3 4/5] daemon/parted: work around part table type misreporting by "parted"
On 11/25/21 11:15, Richard W.M. Jones wrote:> On Thu, Nov 25, 2021 at 10:49:53AM +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 at redhat.com> >> Acked-by: Richard W.M. Jones <rjones at redhat.com> >> --- >> >> Notes: >> v3: >> >> - pick up Rich's ACK >> >> - drop "Unix." namespace selector from "read" [Rich] >> >> - factor "Bytes.get_uint8 sec0" out to "sec0at" [Rich, Laszlo] >> >> v2: >> >> - new patch (the hard one) >> >> daemon/utils.mli | 10 ++++++++++ >> daemon/parted.ml | 7 ++++++- >> daemon/utils.ml | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 61 insertions(+), 1 deletion(-) >> >> 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. *) >> 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..c51a0d181365 100644 >> --- a/daemon/utils.ml >> +++ b/daemon/utils.ml >> @@ -187,6 +187,51 @@ 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 = read fd sec0 0 sec0size in >> + let sec0at = fun offset -> Bytes.get_uint8 sec0 offset in > > I think this should be: > > let sec0at = Bytes.get_uint8 sec0 in > > Otherwise the patch series looks fine to me.The book I've been learning from uses these forms interchangeably. More precisely, it gives the "fun" form as the main one, and then says, "Since named functions are so common, OCaml provides an alternative syntax for functions using a let definition". It provides the following examples, and states that they are equivalent: let sum = fun i j -> i + j;; let sum = (fun i -> (fun j -> i + j));; let sum i j = i + j;; Why is the usage of "fun" awkward here? Is the problem more that I used an explicit "offset" parameter, so sec0at is not defined as a partial function application? Thanks! Laszlo> > Rich. > >> + (* sector read completely *) >> + sec0read = sec0size && >> + >> + (* boot signature present *) >> + sec0at (sigofs ) = 0x55 && >> + sec0at (sigofs + 0x1) = 0xAA && >> + >> + (* mkfs.fat signature present *) >> + Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat" && >> + >> + (* partition bootable *) >> + sec0at (pte1ofs ) = 0x80 && >> + >> + (* partition starts at C/H/S 0/0/1 *) >> + sec0at (pte1ofs + 0x1) = 0x00 && >> + sec0at (pte1ofs + 0x2) = 0x01 && >> + sec0at (pte1ofs + 0x3) = 0x00 && >> + >> + (* partition type is a FAT variant that mkfs.fat is known to create *) >> + List.mem (sec0at (pte1ofs + 0x4)) parttypes && >> + >> + (* partition starts at LBA 0 *) >> + sec0at (pte1ofs + 0x8) = 0x00 && >> + sec0at (pte1ofs + 0x9) = 0x00 && >> + sec0at (pte1ofs + 0xA) = 0x00 && >> + sec0at (pte1ofs + 0xB) = 0x00 >> + ) >> + with _ -> false >> + >> let proc_unmangle_path path >> let n = String.length path in >> let b = Buffer.create n in >> -- >> 2.19.1.3.g30247aa5d201 >> >> >> _______________________________________________ >> Libguestfs mailing list >> Libguestfs at redhat.com >> https://listman.redhat.com/mailman/listinfo/libguestfs >
Richard W.M. Jones
2021-Nov-25 15:46 UTC
[Libguestfs] [PATCH v3 4/5] daemon/parted: work around part table type misreporting by "parted"
On Thu, Nov 25, 2021 at 04:27:42PM +0100, Laszlo Ersek wrote:> It provides the following examples, and states that they are equivalent: > > let sum = fun i j -> i + j;; > let sum = (fun i -> (fun j -> i + j));; > let sum i j = i + j;; > > Why is the usage of "fun" awkward here?They are equivalent, but no one is using the "fun" form in real code because it's longer and more obscure for no reason.> Is the problem more that I used an explicit "offset" parameter, so > sec0at is not defined as a partial function application?I made two changes but didn't explain that well. Firstly get rid of the "fun" as above: let sec0at offset = Bytes.get_uint8 sec0 offset in Secondly, you can drop the argument to produce this equivalent and shorter form: let sec0at = Bytes.get_uint8 sec0 in (https://rosettacode.org/wiki/Partial_function_application#OCaml) 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/