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