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

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 at 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/e7f72ab146b9c2aaee92a600a1fcbefb0202d41c/daemon/isoinfo.ml#L82

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 at 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/e7f72ab146b9c2aaee92a600a1fcbefb0202d41c/daemon/isoinfo.ml#L82
- 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