Re: [libvirt] [PATCHv2 0/3] Xen: Fix <clock> handling
On 02/13/2012 03:36 AM, Philipp Hahn wrote:
Hello Eric,
On Monday 13 February 2012 08:51:41 Philipp Hahn wrote:
> On Thursday 09 February 2012 23:01:38 Eric Blake wrote:
...
>> But a better idea is to represent the XML in a way where the default
>> conversion makes sense, but where a user can explicitly change the XML
>> to get what they want. I'm thinking:
>>
>> If offset is 'utc' or 'localtime', we add a new attribute
'adjustment'
>> ...
>
> I'll give your approch a try, since I think this solves the problem with my
> STRICT.
I encountered some problems while thinking more about your suggestions:
1. The problem here is that the libvirt XML parser (virDomainDefParseString())
is more or less driver agnostic and thus does not know, which variants of
clock/@offset are supported by the driver:
Correct - the domain_conf representation format must specify all
combinations that at least one hypervisor finds useful, and
hypervisor-specific code must then filter out the combinations that they
do not support.
For example since for qemu the XML
is used as the native format, some values are only checked when you run the
domain, that is when the XML in converted to command-line arguments to kvm.
So your extra @reset and @adjustment attributes need to be pased into "struct
_virDomainClockDef".
Yes - that means _virDomainClockDef needs to add new fields to track the
new XML; where zero-initializing the field corresponds to omitting the
attribute.
I see no canonical location to then convert <clock
offset='utc'
adjustment='42'/> to <clock offset='variable'
adjustment='42' basis='utc'/>
for everything but Xen-HV≥3.1, so the check / conversion has to be done for
every single driver.
Yes, this is sometimes the case with adding new XML, that we have to go
back and tweak all of the drivers to cope with the new fields.
2. Instead of implementing a second variant of offset='variable' it would be
enough to just add an attribute reset='true' (or
this_is_not_from_a_buggy_libvirt_and_I_really_need_the_reset='true') to
dumpxml, when a driver really implements the reset semantics. As long as it's
missing, libvirt can convert it to reset='false' in the Xen≥3.1 case. With an
explicit reset='true' libvirt would return an error for Xen-HV≥3.1.
Once we add a new attribute, we can document that domain XML that omits
the attribute will default to hypervisor-specific choices (xen < 3.1 can
default one way, xen >= 3.1 can default another way, and qemu can
default in a particular way as well). If the user provides the
attribute, then they are requesting the specific behavior that they need.
A more general solution would be to add a libvirt-version attribute to the XML
data, which libvirt could use to implement a bug-compatibility-mode: If we're
reading an old XML, treat offset='utc' as offset='variable', but if
it's an
XML from ≥0.9.10, return an error. (how to treat XML without an explicit
version?)
No. We _don't_ want a libvirt version attribute. What we want is a new
attribute that, if present, determines the behavior according to the
user, and if absent, provides sane upgrade semantics when parsing older
XML into newer libvirt.
Yes, I know that the libvirt XML is supposed to be backward
compatible, but
errors happen.
You still haven't managed to convince me that we cannot solve this by
adding new attributes.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)