Re: [Libguestfs] [v2v PATCH 2/9] create_libvirt_xml: simplify match on (s_cpu_vendor, s_cpu_model)

21 Apr 2022

On 04/20/22 18:49, Richard W.M. Jones wrote:
...
On Wed, Apr 20, 2022 at 06:23:26PM +0200, Laszlo Ersek wrote:
...
Squash the patterns
| None, None   -> ()
  | Some _, None -> ()
into the identical
| _, None -> ()
We preserve the behavior added by commit 2a576b7cc5c3 ("v2v: -o libvirt:
Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21);
the change only simplifies the code.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 output/create_libvirt_xml.ml | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index 36173e58cd6c..4e5bbceffabd 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -162,55 +162,57 @@ let create_libvirt_xml ?pool source inspect
(match get_osinfo_id inspect with
    | None -> ()
    | Some osinfo_id ->
      List.push_back_list body [
        e "metadata" [] [
          e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [
            e "libosinfo:os" ["id", osinfo_id] [];
          ];
        ];
      ];
   );
let memory_k = source.s_memory /^ 1024L in
   List.push_back_list body [
     e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)];
     e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)];
     e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
   ];
if source.s_cpu_vendor <> None || source.s_cpu_model <> None ||
      source.s_cpu_topology <> None then (
     let cpu = ref [] in
(match source.s_cpu_vendor, source.s_cpu_model with
-     | None, None
-     (* Avoid libvirt error: "CPU vendor specified without CPU model" *)
-     | Some _, None -> ()
+     | _, None ->
+         (* This also avoids the libvirt error:
+          * "CPU vendor specified without CPU model".
+          *)
+         ()
      | None, Some model ->
         List.push_back cpu (e "model" ["fallback", "allow"] [PCData model])
      | Some vendor, Some model ->
         List.push_back_list cpu [
           e "vendor" [] [PCData vendor];
           e "model" ["fallback", "allow"] [PCData model]
         ]
     );
     (match source.s_cpu_topology with
      | None -> ()
      | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } ->
         let topology_attrs = [
           "sockets", string_of_int s_cpu_sockets;
           "cores", string_of_int s_cpu_cores;
           "threads", string_of_int s_cpu_threads;
         ] in
         List.push_back cpu (e "topology" topology_attrs [])
     );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ]
   );
let uefi_firmware =
     match target_firmware with
     | TargetBIOS -> None
     | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
As an aside, your git includes a crazy large amount of context around
the changes ...  Is that a configuration change?
It was intentional: I passed "-U26" to git-format-patch.

Before I post a patch series, I review each patch in "gitk", and
determine the minimal context size that makes it easy for reviewers to
get a complete understanding of the change, without having to apply the
series, and to re-display the patches locally with a larger context.

Most of the time, the option "--function-context" (aka -W) works well
for this, and that option supports even OCaml -- but only to an extent.
When definitions are nested, and/or introduced with "and" (not "let"),
the function boundaries are not determined well. So in that case, I
start with a standard context size of 3 lines in gitk, and increase it
as needed, as I traverse the patches.

In this case, I arrived at "-U26" because of patch#4:

create_libvirt_xml: eliminate childless <cpu match="minimum"/> element

Because you really need 26 lines of context to see that the patch is
correct.

This doesn't tell you anything:
...
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index d2ca894d60bb..4a71f8bb27f5 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -180,7 +180,7 @@ let create_libvirt_xml ?pool source inspect
     e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
   ];
-  if source.s_cpu_vendor <> None || source.s_cpu_model <> None ||
+  if source.s_cpu_model <> None ||
      source.s_cpu_topology <> None then (
     let cpu = ref [] in
but this does:
...
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index d2ca894d60bb..4a71f8bb27f5 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -157,53 +157,53 @@ let create_libvirt_xml ?pool source inspect
(match source.s_genid with
    | None -> ()
    | Some genid -> List.push_back body (e "genid" [] [PCData genid])
   );
(match get_osinfo_id inspect with
    | None -> ()
    | Some osinfo_id ->
      List.push_back_list body [
        e "metadata" [] [
          e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [
            e "libosinfo:os" ["id", osinfo_id] [];
          ];
        ];
      ];
   );
let memory_k = source.s_memory /^ 1024L in
   List.push_back_list body [
     e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)];
     e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)];
     e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
   ];
-  if source.s_cpu_vendor <> None || source.s_cpu_model <> None ||
+  if source.s_cpu_model <> None ||
      source.s_cpu_topology <> None then (
     let cpu = ref [] in
(match source.s_cpu_model with
      | None -> ()
      | Some model ->
          (match source.s_cpu_vendor with
           | None -> ()
           | Some vendor ->
               List.push_back cpu (e "vendor" [] [PCData vendor])
          );
          List.push_back cpu (e "model" ["fallback", "allow"] [PCData model])
     );
     (match source.s_cpu_topology with
      | None -> ()
      | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } ->
         let topology_attrs = [
           "sockets", string_of_int s_cpu_sockets;
           "cores", string_of_int s_cpu_cores;
           "threads", string_of_int s_cpu_threads;
         ] in
         List.push_back cpu (e "topology" topology_attrs [])
     );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ]
   );
Unfortunately, I don't know of a way to attach this "useful context
size" information to the individual patches; especially so that the info
survive rebases. I could format each patch individually with a different
-U option, but that's something I'm not ready to do :) If I could attach
specially formatted notes, or commit message lines, to the patches, for
driving "-U", that would be really nice.

Also I usually spell out my quirky -U options in the cover letter, but
this time I didn't do it.

Thanks
Laszlo

    