Re: [Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"

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(a)redhat.com&gt; >> --- >> >> 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/e7f72ab146b9c2aaee92a600a1f... - 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.