[libvirt] [PATCHv2 01/21] virCaps: conf: start splitting out irrelevat data

[Laine Stump]

On 03/07/2013 11:50 AM, Daniel P. Berrange wrote:
> On Wed, Mar 06, 2013 at 04:37:45PM +0100, Peter Krempa wrote:
>> The virCaps structure gathered a ton of irrelevant data over time that.
>> The original reason is that it was propagated to the XML parser
>> functions.
>>
>> This patch aims to create a new data structure virDomainXMLConf that
>> will contain immutable data that are used by the XML parser. This will
>> allow two things we need:
>>
>> 1) Get rid of the stuff from virCaps
>>
>> 2) Allow us to add callbacks to check and add driver specific stuff
>> after domain XML is parsed.
>>
>> This first attempt removes pointers to private data allocation functions
>> to this new structure and update all callers and function that require
>> them.
>> ---
>>  src/conf/capabilities.h |  8 ++----
>>  src/conf/domain_conf.c  | 70 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/conf/domain_conf.h  | 27 +++++++++++++++++++
>>  3 files changed, 99 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
>> index cc01765..bf64296 100644
>> --- a/src/conf/capabilities.h
>> +++ b/src/conf/capabilities.h
>> @@ -159,19 +159,15 @@ struct _virCaps {
>>      size_t nguests;
>>      size_t nguests_max;
>>      virCapsGuestPtr *guests;
>> +
>> +    /* deal with these later */
> Better written as /* Move to virDomainXMLConf later */
>
>>      unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN];
>>      unsigned int emulatorRequired : 1;
>>      const char *defaultDiskDriverName;
>>      int defaultDiskDriverType; /* enum virStorageFileFormat */
>>      int (*defaultConsoleTargetType)(const char *ostype, virArch guestarch);
>> -    void *(*privateDataAllocFunc)(void);
>> -    void (*privateDataFreeFunc)(void *);
>> -    int (*privateDataXMLFormat)(virBufferPtr, void *);
>> -    int (*privateDataXMLParse)(xmlXPathContextPtr, void *);
>>      bool hasWideScsiBus;
>>      const char *defaultInitPath;
>> -
>> -    virDomainXMLNamespace ns;
>>  };
>>
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f7c8af1..2b54aec 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -736,6 +736,76 @@ static int virDomainObjOnceInit(void)
>>
>>  VIR_ONCE_GLOBAL_INIT(virDomainObj)
>>
>> +
>> +/* This structure holds various callbacks and data needed
>> + * while parsing and creating domain XMLs */
>> +struct _virDomainXMLConf {
>> +    virObject parent;
>> +
>> +    /* domain private data management callbacks */
>> +    virDomainXMLPrivateDataAllocFunc  privateDataAllocFunc;
>> +    virDomainXMLPrivateDataFreeFunc   privateDataFreeFunc;
>> +    virDomainXMLPrivateDataFormatFunc privateDataXMLFormat;
>> +    virDomainXMLPrivateDataParseFunc  privateDataXMLParse;
> Why not just use  'virDomainXMLPrivateDataCallbacks' here
> instead of duplicating its contents ?
>
>> +
>> +    /* XML namespace callbacks */
>> +    virDomainXMLNamespace ns;
>> + };
> Good, I like that this struct is only in the .c file and not
> exposed in .h
>
>
>> +virDomainXMLConfPtr
>> +virDomainXMLConfNew(virDomainXMLPrivateDataCallbacksPtr priv,
>> +                    virDomainXMLNamespacePtr xmlns)
>> +{
>> +    virDomainXMLConfPtr xmlconf;
>> +
>> +    if (virDomainXMLConfInitialize() < 0)
>> +        return NULL;
>> +
>> +    if (!(xmlconf = virObjectNew(virDomainXMLConfClass)))
>> +        return NULL;
>> +
>> +    if (priv) {
>> +        xmlconf->privateDataAllocFunc = priv->alloc;
>> +        xmlconf->privateDataFreeFunc = priv->free;
>> +        xmlconf->privateDataXMLFormat = priv->format;
>> +        xmlconf->privateDataXMLParse = priv->parse;
> Then you could just do
>
>   if (priv)
>      memcpy(xmlconf->privDataCallbacks, priv, sizeof(*priv));

Or use struct assignment:

      xmlconf->privDataCallbacks = *priv;

(I have an innate dislike of memcpy when it can be avoided, since it
eliminates type checking).

>
>> +    }
>> +
>> +    if (xmlns)
>> +        xmlconf->ns = *xmlns;


Yeah, like you did there.

>> +
>> +    return xmlconf;
>> +}
>> +
>> +virDomainXMLNamespace
>> +virDomainXMLConfGetNamespace(virDomainXMLConfPtr xmlconf)
>> +{
>> +    return xmlconf->ns;
>> +}

You're returning a struct there, rather than a pointer to a struct. Do
you really want to restrict yourself that way?