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

[Laszlo Ersek]

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