Libguestfs - Dec 2021 - [PATCH] convert_linux: translate the first CD-ROM's references in boot conf files

On 12/14/21 21:46, Richard W.M. Jones wrote:
> On Tue, Dec 14, 2021 at 04:17:49PM +0100, Laszlo Ersek wrote:
>> +    match cdroms with
>> +    | _ :: _ :: _ -> warning (f_"multiple CD-ROMs found;
translation of \
>> +                                 CD-ROM references may be
inexact")
>> +    | _ -> ();
>> +
>> +    let map = map @
> 
> This part is either wrong or indented incorrectly.  The "let map
..."
> part is part of the second branch of the match statement and doesn't
> run if the warning is printed.
*groan* I admit the binding strengths of these operators keep confusing me.

(Also, I meant to post this as an RFC in the first round...)
> 
> In any case it's probably better to replace the confusing first match
> with something simpler such as:
> 
>   if List.length cdroms > 2 then
>     warning (f_"multiple CD-ROMs found; translation of CD-ROM
references may be inexact");
Yes, this was my original version, but I figured not counting the full
length (until the end of the list) is more welcome. I can update it though.
> 
> The \ is also not necessary in the original since OCaml lets you have
> arbitrary length strings over multiple lines (a good thing),
I'm aware, however. :)

I *really* dislike the one aspect of the current multi-line messages
that they are nailed to column #0. It disrupts the code flow for me. For
example, consider the following snippet from this very file
[convert/convert_linux.ml]:

                g#cp grub_config uefi_grub_conf;
                let fix_script = sprintf
"#!/bin/bash
efibootmgr -c -L \"CentOS 6\"
rm -rf %s" uefi_fallback_path in
                Firstboot.add_firstboot_script
                  g inspect.i_root "fix uefi boot" fix_script)

I find it less than ideal.

I'd really like to indent such strings logically. And that's exactly
what the backslash is for (I had checked the ocaml documentation): after
an escaped newline, leading spaces and tabs are dropped. I think that's
a fantastic feature of OCaml; I used it intentionally.
> plus we
> automatically wrap warning and error strings on whitespace.
That sounds very user friendly (I didn't expect it!), but does not
contradict my point about formatting the source code, AFAICT.
> 
>> +      (match cdroms with
>> +       | cdrom :: _ ->
>> +           (match (cdrom.s_removable_controller,
cdrom.s_removable_slot,
>> +                   family, inspect.i_major_version) with
>> +            | Some Source_IDE, Some slot, `RHEL_family, v when v <=
5 ->
> 
> You can just match to arbitrary depth in single match statements, so
> the whole match would become something like:
> 
>   let map = map @
>     match cdroms with
>     | { Some Source_IDE, Some slot, `RHEL_family v } :: _ when v <= 5
->
>            [("hd" ^ drive_name slot, "cdrom")]
>     | _ -> [] in
Does this make the code easier to read to you? (Honest question.) To me,
this does not separate the steps "take first" and "investigate
first",
and so it's harder to read. But if that's only because I don't have
enough OCaml experience yet, I'm happy to change it. (Because, in
comparison, you might find the two "match" expressions to be the
distraction.)

Thanks!
Laszlo

On Wed, Dec 15, 2021 at 11:17:51AM +0100, Laszlo Ersek
wrote:
> I *really* dislike the one aspect of the current multi-line messages
> that they are nailed to column #0. It disrupts the code flow for me. For
> example, consider the following snippet from this very file
> [convert/convert_linux.ml]:
> 
>                 g#cp grub_config uefi_grub_conf;
>                 let fix_script = sprintf
> "#!/bin/bash
> efibootmgr -c -L \"CentOS 6\"
> rm -rf %s" uefi_fallback_path in
>                 Firstboot.add_firstboot_script
>                   g inspect.i_root "fix uefi boot" fix_script)
>
> I find it less than ideal.
> 
> I'd really like to indent such strings logically. And that's
exactly
> what the backslash is for (I had checked the ocaml documentation): after
> an escaped newline, leading spaces and tabs are dropped. I think that's
> a fantastic feature of OCaml; I used it intentionally.
Right, this is indeed a reason to use the \, as it drops the following
whitespace.
> > You can just match to arbitrary depth in single match statements, so
> > the whole match would become something like:
> > 
> >   let map = map @
> >     match cdroms with
> >     | { Some Source_IDE, Some slot, `RHEL_family v } :: _ when v <=
5 ->
> >            [("hd" ^ drive_name slot, "cdrom")]
> >     | _ -> [] in
> 
> Does this make the code easier to read to you? (Honest question.) To me,
> this does not separate the steps "take first" and
"investigate first",
> and so it's harder to read. But if that's only because I don't
have
> enough OCaml experience yet, I'm happy to change it. (Because, in
> comparison, you might find the two "match" expressions to be the
> distraction.)
For me it's less code => better, within reason. The above (after
fixing the bugs - see later emails) is more natural.

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