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:
> +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