[Libguestfs] [v2v PATCH 2/9] create_libvirt_xml: simplify match on (s_cpu_vendor, s_cpu_model)
Richard W.M. Jones
rjones at redhat.com
Thu Apr 21 16:56:06 UTC 2022
On Thu, Apr 21, 2022 at 05:27:06PM +0200, Laszlo Ersek wrote:
> 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 at 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 at 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.
Understood - I think it does improve the ability to review patches.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
More information about the Libguestfs
mailing list