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

[Peter Krempa <pkrempa@xxxxxxxxxx>]

[...]

So your series went through rather messed up. I see at least two
different copies of patch 2/something.

On Thu, Feb 08, 2018 at 14:45:59 +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

So this commit message should describe what this patch is using, but
it's not.

> ---
>  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

We use only old-style comments.

> +    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);

Typo in the name and also you should not typedef in the functions.

> +
>      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++;
> +    }

NACK to this. I prefer if the functions are visible in the code in the
correct order, so please don't try to load them from an array of
callbacks. I don't see any benefit of this.

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list