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