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

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.

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");

The \ is also not necessary in the original since OCaml lets you have
arbitrary length strings over multiple lines (a good thing), plus we
automatically wrap warning and error strings on whitespace.
> +      (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

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, Dec 14, 2021 at 08:46:45PM +0000, 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.
> 
> In any case it's probably better to replace the confusing first match
> with something simpler such as:
> 
>   if List.length cdroms > 2 then
">=" ?  I said it was confusing :-)

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 Tue, Dec 14, 2021 at 08:46:45PM +0000, 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.
> 
> 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");
> 
> The \ is also not necessary in the original since OCaml lets you have
> arbitrary length strings over multiple lines (a good thing), plus we
> automatically wrap warning and error strings on whitespace.
> 
> > +      (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
->
This line should have been:

 | { s_removable_slot = Some Source_IDE; s_removable_slot = Some slot } :: _
      when family = `RHEL_family && inspect.i_major_version <= 5
->

Rich.
>            [("hd" ^ drive_name slot, "cdrom")]
>     | _ -> [] in
> 
> Rich.
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> libguestfs lets you edit virtual machines.  Supports shell scripting,
> bindings from many languages.  http://libguestfs.org
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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