[libvirt] [PATCH v1 01/51] util: conf: separate virDomainDefParseXML into serial parts
Peter Krempa
pkrempa at redhat.com
Thu Feb 8 09:12:12 UTC 2018
[...]
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](¶m) < 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180208/48a63fb1/attachment-0001.sig>
More information about the libvir-list
mailing list