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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list