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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
