Re: [Libguestfs] [PATCH v2v 3/5] -o libvirt: Add <clock offset="utc|localtime"> to libvirt XML

On 9/25/23 16:04, Richard W.M. Jones wrote: > --- > output/create_libvirt_xml.ml | 10 ++++++++-- > tests/test-v2v-i-ova.xml | 1 + > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml > index 964acd25fd..f97272ca31 100644 > --- a/output/create_libvirt_xml.ml > +++ b/output/create_libvirt_xml.ml > @@ -292,10 +292,16 @@ let create_libvirt_xml ?pool source inspect > e "nvram" ["template", vars_template] [] ] in > > List.push_back_list os loader; > - !os in > + e "os" [] !os in > + > + (* The <clock> section. *) > + let clock_section = > + let offset = if guestcaps.gcaps_bios_utc then "utc" else "localtime" in > + e "clock" [ "offset", offset ] [] in > > List.push_back_list body [ > - e "os" [] os_section; > + os_section; > + clock_section; > > e "on_poweroff" [] [PCData "destroy"]; > e "on_reboot" [] [PCData "restart"]; I don't think we need to change what "os_section" is bound to, here; I understand the idea is to increase consistency, but for me it makes the patch harder to read -- ultimately it is a refactoring, and then adding in clock_section is the new thing. If you can split these apart, that's optimal; if not, this is viable too IMO.

Thanks Laszlo > diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml > index e5907ea1cc..a41827bfd5 100644 > --- a/tests/test-v2v-i-ova.xml > +++ b/tests/test-v2v-i-ova.xml > @@ -18,6 +18,7 @@ > <os> > <type arch='x86_64' machine='q35'>hvm</type> > </os> > + <clock offset='utc'/> > <on_poweroff>destroy</on_poweroff> > <on_reboot>restart</on_reboot> > <on_crash>restart</on_crash>