[libvirt] [PATCH v1 01/51] util: conf: separate virDomainDefParseXML into serial parts

[Daniel P. Berrangé]

On Thu, Feb 08, 2018 at 02:45:59PM +0800, xinhua.Cao wrote:
> beacause virDomainDefParseXML is too long, and all domain xml
> parse in this function, it's difficulty to maintain this function
> so separate virDomainDefParseXML into serial parts use
> virDomainPreaseInfoFunc. then it will easy to maintain
> ---
>  src/conf/domain_conf.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 34aae82..e36783b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18527,6 +18527,28 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>  }
>  
>  
> +typedef struct _virDomainParseTotalParam virDomainParseTotalParam;
> +typedef virDomainParseTotalParam *virDomainParseTotalParamPtr;
> +
> +struct _virDomainParseTotalParam{
> +    //input parameters
> +    virDomainDefPtr def;
> +    xmlDocPtr xml;
> +    xmlNodePtr root;
> +    xmlXPathContextPtr ctxt;
> +    virCapsPtr caps;
> +    virDomainXMLOptionPtr xmlopt;
> +    void *parseOpaque;
> +    unsigned int flags;
> +
> +    //internal parameters
> +    bool usb_none;
> +    bool usb_other;
> +    bool usb_master;
> +    bool uuid_generated;
> +};
> +
> +
>  static virDomainDefPtr
>  virDomainDefParseXML(xmlDocPtr xml,
>                       xmlNodePtr root,
> @@ -18536,11 +18558,14 @@ virDomainDefParseXML(xmlDocPtr xml,
>                       void *parseOpaque,
>                       unsigned int flags)
>  {
> +    typedef int (*virDomainPreaseInfoFunc)(virDomainParseTotalParamPtr params);
> +
>      xmlNodePtr *nodes = NULL, node = NULL;
>      char *tmp = NULL;
>      size_t i, j;
>      int n, virtType, gic_version;
>      long id = -1;
> +    size_t fun_index = 0;
>      virDomainDefPtr def;
>      bool uuid_generated = false;
>      virHashTablePtr bootHash = NULL;
> @@ -18548,6 +18573,25 @@ virDomainDefParseXML(xmlDocPtr xml,
>      bool usb_other = false;
>      bool usb_master = false;
>      char *netprefix = NULL;
> +    virDomainParseTotalParam param = {
> +            NULL,
> +            xml,
> +            root,
> +            ctxt,
> +            caps,
> +            xmlopt,
> +            parseOpaque,
> +            flags,
> +            false,
> +            false,
> +            false,
> +            false
> +
> +    };
> +
> +    virDomainPreaseInfoFunc parse_funs[] = {
> +            NULL
> +    };
>  
>      if (flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA) {
>          char *schema = virFileFindResource("domain.rng",
> @@ -18565,6 +18609,14 @@ virDomainDefParseXML(xmlDocPtr xml,
>      if (!(def = virDomainDefNew()))
>          return NULL;
>  
> +    param.def = def;
> +
> +    while (parse_funs[fun_index]) {
> +        if (parse_funs[fun_index](&param) < 0)
> +            goto error;
> +        fun_index++;
> +    }

I don't really like this approach. I prefer to see the separate functions
called explicitly by name, and just passing in the particular parameters
that each function actually needs.

IOW, just drop this patch entirely and update following patches so that
their split function is called.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|