[libvirt] [PATCH 10/16] conf: Add separate defaults addition and validation for XML parsing

[Peter Krempa]

On 03/01/13 20:10, Laine Stump wrote:
> On 02/20/2013 12:06 PM, 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.
>>
>> Two callback pointers are exposed in the virCaps object:
>> * virDriverDeviceDefCallback - called for a single device parsed and for
>>                                 every single device in a domain config.
>>                                 A virDomainDeviceDefPtr is passed.
>> * virDriverDomainDefCallback - called if a domain definition is parsed
>>                                 device specific callbacks were called.
>>                                 A virDomainDefPtr is passed.
>>
>> Errors may be reported in those callbacks resulting in a XML parsing
>> failure.
>>
>> Additionally two internal filler functions are added:
>> virDomainDeviceDefUpdateDefaultsInternal and
>> virDomainDefUpdateDefaultsInternal that are meant to be used for
>> separating the validation and defaults assignment code from the parser
>> function.
>> ---
>>   src/conf/capabilities.h |  6 +++++
>>   src/conf/domain_conf.c  | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 78 insertions(+)
>>
>> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
>> index cc01765..56cd09f 100644
>> --- a/src/conf/capabilities.h
>> +++ b/src/conf/capabilities.h
>> @@ -171,6 +171,12 @@ struct _virCaps {
>>       bool hasWideScsiBus;
>>       const char *defaultInitPath;
>>
>> +
>> +    /* these callbacks are called after a XML definition of a device or domain
>> +     * is parsed. They are meant to validate and fill driver-specific values. */
>> +    int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */
>> +    int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */
>
> If you know that  virDriverDeviceDefCallback is always given a
> virDomainDeviceDefPtr, why prototype it as void*? Same question for
> virDriverDomainDefCallback.

Those two types are defined in conf/domain_conf.h. conf/domain_conf.h in 
return needs conf/capabilities.h. This creates a circular dependency 
chain if I try to include domain_conf.h in capabilities.h. I tried to to 
come up with a different solution than void * but none of the others 
were nicer than that. The options were:

1) use a separate header file for one of the types
2) use a extern declaration
3) include domain_conf.h into capabilities.h after all needed types were 
declared.

I'll welcome any tips how to solve this problem. (cc-d Dan and Eric).

>
> Additionally, in the callback for a device, we will need to have the
> virDomainDefPtr (e.g. so that we can see what machinetype was selected
> for the domain). And in both of these callbacks we will need to get the
> virCapsPtr so that (for example), we can have access to information
> about which devices are available for which machine types, the default
> pci addresses of integrated devices, etc.
>
> So, I think the callbacks should be like this:
>
>    int (*virDriverDeviceDefCallback) (virCapsPtr caps, virDomainDefPtr
> *domdef, virDomainDeviceDefPtr *devdef);
>    int (*virDriverDomainDefCallback) (virCapsPtr caps, virDomainDefPtr
> *domdef);
>

Yep, seems reasonable to me.

Peter