[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@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@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@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
On 04/11/22 10:13, Richard W.M. Jones wrote:
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@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.
I'll close the BZ as NOTABUG. I don't think we should complicate the code for hard-to-justify feature requests. We have enough actual issues. I wrote this patch because it was easier to approach the bug report like this than to start arguing (again) that the change is unnecessary. If the bug reporter insists, we can still merge this later. Thanks Laszlo
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@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones