10:27 a.m.
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>
> ---
> 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>
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