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

If this information is useful, please help other people find it:
Share via:

Laszlo Ersek

2021-Nov-25 08:54 UTC

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

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

Richard W.M. Jones

2021-Nov-25 10:10 UTC

head link

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

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/24fc04eb97d718b4d8514a1ac075b54c0e6b1402?branch=rawhide
>       (* 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

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

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

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