4:10 a.m.
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/24fc04eb97d718b4d8514a1ac...
(* 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