Re: [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