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

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(a)redhat.com&gt; >> --- >> 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(a)redhat.com&gt; > > 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.