[libvirt] [PATCHv2 15/21] conf: Add separate defaults addition and validation for XML parsing

[Laine Stump]

On 03/07/2013 12:43 PM, Peter Krempa wrote:
> On 03/07/13 18:34, Laine Stump wrote:
>> On 03/06/2013 10:37 AM, Peter Krempa wrote:
>>> This patch adds instrumentation that will ultimately allow to split out
>>> filling of defaults and input validation from the XML parser to
>>> separate
>>> functions.
>>>
>>> With this patch, after the XML is parsed, a callback to the driver is
>>> issued requesing to fill and validate driver specific details of the
>>> configuration. This allows to use sensible defaults and checks on a per
>>> driver basis at the time the XML is parsed.
>>>
>>> Three callback pointers are exposed in the new virDomainXMLConf object:
>>> * virDeviceDefAdjustCallback - called for a single device parsed and
>>>                                 for every single device in a domain
>>>                                 config. A virDomainDeviceDefPtr is
>>>                                 passed along with the domain
>>>                                 definition and virCaps.
>>> * virDomainDefAdjustCallback - called if a domain definition is parsed
>>>                                 device specific callbacks were called.
>>>                                 A virDomainDefPtr is passed along with
>>>                                 virCaps. There are two callbacks of
>>> this
>>>                                 kind, one called before the devices are
>>>                                 parsed and one after.
>>>
>>> Errors may be reported in those callbacks resulting in a XML parsing
>>> failure.
>>>
>>> Additionally internal filler and checker functions are added that are
>>> meant to be used forseparating the validation and defaults assignment
>>> code from the parser function.
>>
>> Just repeating what I said in another mail on a different (and probably
>> buried) thread yesterday:
>>
>>     https://www.redhat.com/archives/libvir-list/2013-March/msg00249.html
>>
>> I think that not all validation should be done in these separate
>> callbacks. Only validation that requires more information than is
>> available in the currently-in-process element (e.g., needs to know
>> hypervisor-specific or host environment-specific information, or
>> something about a completely different part of the domain xml) should be
>> done externally; a good indicator of this is if a restriction can't be
>> reasonably included in the RNG, then it probably needs to be validated
>> in the external callback function.
>>
>> If, on the other hand, it is specified in the RNG, and applies to all
>> situations (different hypervisor, hypervisor binary, host, etc), then it
>> should simply be part of the basic parser.
>
> I agree in the general with this. My approach isn't to put all the
> stuff into the driver callbacks. On the other hand, I'd like to keep
> the parser function tidy and the checks don't help in this effort.

Define "tidy". When tracing through the code trying to figure out what's
happening, it's often much simpler if everything related to a particular
field happens in the same place, rather than needing to figure out about
separate callback functions, or post-processing functions that are
called later. In the most degenerate case, you could end up with
completely parallel function hierarchies for parsing and validation.

> That's why I created verification functions (see patch 21) that is
> there to do all the common checking stuff.

The fixup done in that patch (demanding that os.init be present when
os.type == "exe", and setting it to "/sbin/init" if it isn't set) is
kind of a gray area - all the info is available in the immediate object
being parsed, but on the other hand it is a hypervisor-specific element
(no other os types use the <init> element). So, while I definitely agree
with that patch that setting a default value should be done in the
openvz-specific callback, I'm conflicted about whether mandating its
presence be done in the parser or in the openvz callback.

... Ah, now I can see why you needed the separate validation function in
this case, though - you want to require that init be set, but you can't
check for that until after you've finished parsing and called the
hypervisor's post-parse callback...

Okay, I've decided to agree that the separate validation function is a
reasonable thing and has good use cases, but think that its use should
be clearly defined and kept to a minimum (and comments placed in the
code to indicate such).

(BTW, I looked in the RNG and see that, although the parser requires
<init> when os.type == "exe", the RNG says that it's optional in that
case (and not allowed when os.type is anything different)