[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv1 03/13] Separate out virStorageFeatureParse



On Fri, Apr 10, 2015 at 14:58:55 +0200, Ján Tomko wrote:
> To allow sharing the function between snapshot_conf and storage_conf.
> ---
>  po/POTFILES.in                  |  1 +
>  src/Makefile.am                 |  5 ++++
>  src/conf/storage_conf.c         | 32 ++++-----------------
>  src/conf/storage_feature_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++
>  src/conf/storage_feature_conf.h | 22 +++++++++++++++
>  5 files changed, 96 insertions(+), 26 deletions(-)
>  create mode 100644 src/conf/storage_feature_conf.c
>  create mode 100644 src/conf/storage_feature_conf.h
> 

...

> diff --git a/src/Makefile.am b/src/Makefile.am
> index 91a4c17..4e8679a 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -309,6 +309,10 @@ NWFILTER_CONF_SOURCES =						\

>  		conf/interface_conf.c conf/interface_conf.h
> @@ -342,6 +346,7 @@ CONF_SOURCES =							\
>  		$(NWFILTER_CONF_SOURCES)			\
>  		$(NODE_DEVICE_CONF_SOURCES)			\
>  		$(STORAGE_CONF_SOURCES)				\
> +		$(STORAGE_FEATURE_CONF_SOURCES)     \

The backslash at the end is not indented with tabs thus it's improperly
aligned when viewed with the git tool that formats 8 spaces for tabs.

>  		$(INTERFACE_CONF_SOURCES)			\
>  		$(SECRET_CONF_SOURCES)				\
>  		$(CPU_CONF_SOURCES)				\
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 339f08f..a5b5c1b 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -36,6 +36,7 @@
>  #include "virerror.h"
>  #include "datatypes.h"
>  #include "storage_conf.h"
> +#include "storage_feature_conf.h"
>  #include "virstoragefile.h"
>  
>  #include "virxml.h"
> @@ -1255,9 +1256,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>      char *unit = NULL;
>      char *backingStore = NULL;
>      xmlNodePtr node;
> -    xmlNodePtr *nodes = NULL;
> -    size_t i;
> -    int n;
>  
>      virCheckFlags(VIR_VOL_XML_PARSE_NO_CAPACITY |
>                    VIR_VOL_XML_PARSE_OPT_CAPACITY, NULL);
> @@ -1392,31 +1390,13 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>      if (virXPathNode("./target/nocow", ctxt))
>          ret->target.nocow = true;
>  
> -    if (virXPathNode("./target/features", ctxt)) {
> -        if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0)
> -            goto error;
> -
> -        if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0)
> -            goto error;
> -
> -        if (!(ret->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST)))
> -            goto error;
> -
> -        for (i = 0; i < n; i++) {
> -            int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name);
> -
> -            if (f < 0) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"),
> -                               (const char*)nodes[i]->name);
> -                goto error;
> -            }
> -            ignore_value(virBitmapSetBit(ret->target.features, f));
> -        }
> -        VIR_FREE(nodes);
> -    }
> +    if (virStorageFeaturesParse(ctxt,
> +                                "./target/features",
> +                                &ret->target.compat,
> +                                &ret->target.features) < 0)
> +        goto error;
>  
>   cleanup:
> -    VIR_FREE(nodes);
>      VIR_FREE(allocation);
>      VIR_FREE(capacity);
>      VIR_FREE(unit);
> diff --git a/src/conf/storage_feature_conf.c b/src/conf/storage_feature_conf.c
> new file mode 100644
> index 0000000..77e6406
> --- /dev/null
> +++ b/src/conf/storage_feature_conf.c
> @@ -0,0 +1,62 @@
> +/*
> + * storage_feature_conf.c: config handling for storage file features
> + *
> + * Copyright: Red Hat, Inc
> + *
> + * LGLPv2.1+
> + */

I like this compact header, but I'm not sure if the rest of upstream has
the same opinion.


> +
> +#include <config.h>
> +
> +#include "storage_feature_conf.h"
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virstoragefile.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_STORAGE
> +
> +int virStorageFeaturesParse(xmlXPathContextPtr ctxt,

return type on the same line

> +                            const char *xpath,
> +                            char **compat,
> +                            virBitmapPtr *features)
> +{
> +    xmlNodePtr *nodes = NULL;
> +    char *feat_xpath = NULL;
> +    size_t i;
> +    int n;
> +    int ret = -1;
> +
> +    if (!virXPathNode(xpath, ctxt))
> +        return 0;
> +
> +    if (!*compat && VIR_STRDUP(*compat, "1.1") < 0)
> +        return -1;

Is there a specific reason that you check whether the compat string is
not assigned previously?

> +
> +    if (virAsprintf(&feat_xpath, "%s/*", xpath) < 0)
> +        goto cleanup;
> +
> +    if ((n = virXPathNodeSet(feat_xpath, ctxt, &nodes)) < 0)
> +        goto cleanup;
> +
> +    if (!(*features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST)))
> +        goto cleanup;
> +
> +    for (i = 0; i < n; i++) {
> +        int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name);
> +
> +        if (f < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"),
> +                           (const char*)nodes[i]->name);
> +            goto cleanup;
> +        }
> +        ignore_value(virBitmapSetBit(*features, f));
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(nodes);
> +    VIR_FREE(feat_xpath);
> +    return ret;
> +}
> diff --git a/src/conf/storage_feature_conf.h b/src/conf/storage_feature_conf.h
> new file mode 100644
> index 0000000..a411b66
> --- /dev/null
> +++ b/src/conf/storage_feature_conf.h
> @@ -0,0 +1,22 @@
> +/*
> + * storage_feature_conf.h: config handling for storage file features
> + *
> + * Copyright: Red Hat, Inc
> + *
> + * LGLPv2.1+
> + */
> +
> +#ifndef __VIR_STORAGE_FEATURE_CONF_H__
> +# define __VIR_STORAGE_FEATURE_CONF_H__
> +
> +# include "internal.h"
> +
> +# include "virbitmap.h"
> +# include "virxml.h"
> +
> +int virStorageFeaturesParse(xmlXPathContextPtr ctxt,
> +                            const char *xpath,
> +                            char **compat,
> +                            virBitmapPtr *features);

All four arguments are ATTRIBUTE_NONNULL.

> +
> +#endif /* __VIR_STORAGE_FEATURE_CONF_H__ */

The code looks okay, but I'm not entirely sure about the licence
section, so I'd rather have another opinion.

Peter

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]