Libguestfs - Apr 2022 - [v2v PATCH] convert: warn about "--mac" options that don't match any source NICs

Just before we call "Networks.map", collect all "--mac"
references (from
"options.network_map" and "options.static_ips"), and check
each against
the list of MACs available on the source ("source.s_nics"). Collect
and
report unresolved references.

The algorithm added here has quadratic time complexity, kind of undoing
the sophistication of the "Networks.t" type -- but I don't think
we expect
a huge number of NICs / MACs.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1685809
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 lib/networks.mli   |  3 +++
 convert/convert.ml | 19 +++++++++++++++++++
 lib/networks.ml    |  2 ++
 3 files changed, 24 insertions(+)

diff --git a/lib/networks.mli b/lib/networks.mli
index 8800e21cbcf0..d1a62bf0cf2e 100644
--- a/lib/networks.mli
+++ b/lib/networks.mli
@@ -55,3 +55,6 @@ val map : t -> Types.source_nic -> Types.source_nic
     MAC address mappings take precedence, followed by network
     and bridge mappings if no MAC address mapping for the NIC can
     be found. *)
+
+val macs : t -> string list
+(** Return all MAC addresses for which address mappings have been added. *)
diff --git a/convert/convert.ml b/convert/convert.ml
index 87fca7252ba3..1e3db6b99780 100644
--- a/convert/convert.ml
+++ b/convert/convert.ml
@@ -47,6 +47,25 @@ type mpstat = {
 }
 
 let rec convert dir options source +  let nic_macs = List.filter_map
+                   (fun { s_mac } ->
+                    match s_mac with
+                    | None -> None
+                    | Some mac -> Some (String.lowercase_ascii mac))
+                   source.s_nics
+  and mac_refs1 = Networks.macs options.network_map
+  and mac_refs2 = List.map
+                    (fun { if_mac_addr } -> String.lowercase_ascii
if_mac_addr)
+                    options.static_ips in
+  let unresolved_mac_refs +    List.filter (fun mac_ref -> not (List.mem
mac_ref nic_macs))
+      (mac_refs1 @ mac_refs2) in
+  if unresolved_mac_refs <> [] then (
+    let mac_list = String.concat ", " unresolved_mac_refs in
+    warning (f_"The following --mac addresses do not match any NICs from
the \
+                source: %s") mac_list
+  );
+
   let target_nics = List.map (Networks.map options.network_map) source.s_nics
in
 
   message (f_"Opening the source");
diff --git a/lib/networks.ml b/lib/networks.ml
index 93250fe40ab0..52646166e496 100644
--- a/lib/networks.ml
+++ b/lib/networks.ml
@@ -103,3 +103,5 @@ let add_default_bridge t o    if t.default_bridge <>
None then
     error (f_"duplicate -b/--bridge parameter.  Only one default mapping
is allowed.");
   t.default_bridge <- Some o
+
+let macs t = StringMap.keys t.macs
-- 
2.19.1.3.g30247aa5d201

On Sun, Apr 10, 2022 at 03:49:51PM +0200, Laszlo Ersek
wrote:
> Just before we call "Networks.map", collect all "--mac"
references (from
> "options.network_map" and "options.static_ips"), and
check each against
> the list of MACs available on the source ("source.s_nics").
Collect and
> report unresolved references.
> 
> The algorithm added here has quadratic time complexity, kind of undoing
> the sophistication of the "Networks.t" type -- but I don't
think we expect
> a huge number of NICs / MACs.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1685809
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Looking at this properly, I think the whole bug is dubious to me.

If the user specified a --mac option then we should probably assume
they know more about the source or target than we do.  Or maybe they
are adding a group of --mac command line parameters to every virt-v2v
invocation, which each apply to a subset of the guests being
converted.

Personally I would be inclined close this one as NOTABUG, but as it's
just a warning (and has no effect on anything) I guess it's OK.

Be nice if it wasn't quadratic though.  I was looking at
/usr/lib64/ocaml/map.mli to see if there's any operation we can use to
intersect the map keys with a List or Set, but I cannot see anything ...

Lukewarm ACK.

Rich.
>  lib/networks.mli   |  3 +++
>  convert/convert.ml | 19 +++++++++++++++++++
>  lib/networks.ml    |  2 ++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/lib/networks.mli b/lib/networks.mli
> index 8800e21cbcf0..d1a62bf0cf2e 100644
> --- a/lib/networks.mli
> +++ b/lib/networks.mli
> @@ -55,3 +55,6 @@ val map : t -> Types.source_nic -> Types.source_nic
>      MAC address mappings take precedence, followed by network
>      and bridge mappings if no MAC address mapping for the NIC can
>      be found. *)
> +
> +val macs : t -> string list
> +(** Return all MAC addresses for which address mappings have been added.
*)
> diff --git a/convert/convert.ml b/convert/convert.ml
> index 87fca7252ba3..1e3db6b99780 100644
> --- a/convert/convert.ml
> +++ b/convert/convert.ml
> @@ -47,6 +47,25 @@ type mpstat = {
>  }
>  
>  let rec convert dir options source > +  let nic_macs = List.filter_map
> +                   (fun { s_mac } ->
> +                    match s_mac with
> +                    | None -> None
> +                    | Some mac -> Some (String.lowercase_ascii mac))
> +                   source.s_nics
> +  and mac_refs1 = Networks.macs options.network_map
> +  and mac_refs2 = List.map
> +                    (fun { if_mac_addr } -> String.lowercase_ascii
if_mac_addr)
> +                    options.static_ips in
> +  let unresolved_mac_refs > +    List.filter (fun mac_ref -> not
(List.mem mac_ref nic_macs))
> +      (mac_refs1 @ mac_refs2) in
> +  if unresolved_mac_refs <> [] then (
> +    let mac_list = String.concat ", " unresolved_mac_refs in
> +    warning (f_"The following --mac addresses do not match any NICs
from the \
> +                source: %s") mac_list
> +  );
> +
>    let target_nics = List.map (Networks.map options.network_map)
source.s_nics in
>  
>    message (f_"Opening the source");
> diff --git a/lib/networks.ml b/lib/networks.ml
> index 93250fe40ab0..52646166e496 100644
> --- a/lib/networks.ml
> +++ b/lib/networks.ml
> @@ -103,3 +103,5 @@ let add_default_bridge t o >    if t.default_bridge
<> None then
>      error (f_"duplicate -b/--bridge parameter.  Only one default
mapping is allowed.");
>    t.default_bridge <- Some o
> +
> +let macs t = StringMap.keys t.macs
> -- 
> 2.19.1.3.g30247aa5d201
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
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