8:55 a.m.
I unfortunately looked at this function. I did not like what I've seen.
Let's start making it a bit more bearable.
This series starts splitting and cleaning up virDomainDiskDefParseXML which is
an unmaintainable spaghettti-code mess currently.
Peter Krempa (10):
util: Rename and move virStrIsPrint to virStringIsPrintable
conf: disk: Don't initialize fields allocated by calloc
conf: disk: Remove one unnecessary level of indentation
conf: disk: Extract iotune parsing into a separate func
conf: disk: Remove error label from virDomainDiskDefIotuneParse
conf: virDomainDiskDefIotuneParse: simplfiy parsing
conf: virDomainDiskDefIotuneParse: Report malformed number errors
conf: disk: Split out parsing of disk mirror data
conf: Refactor virDomainDiskDefMirrorParse
conf: extract disk geometry parsing code
src/conf/domain_conf.c | 777 ++++++++++++++++++++++-------------------------
src/libvirt_private.syms | 2 +-
src/util/virstring.c | 18 ++
src/util/virstring.h | 2 +
src/util/virutil.c | 12 -
src/util/virutil.h | 2 -
6 files changed, 377 insertions(+), 436 deletions(-)
--
2.8.0
8:55 a.m.
New subject: [libvirt] [PATCH 01/10] util: Rename and move virStrIsPrint to virStringIsPrintable
---
src/conf/domain_conf.c | 4 ++--
src/libvirt_private.syms | 2 +-
src/util/virstring.c | 18 ++++++++++++++++++
src/util/virstring.h | 2 ++
src/util/virutil.c | 12 ------------
src/util/virutil.h | 2 --
6 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d2cf8d5..0320691 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7155,7 +7155,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
- if (!virStrIsPrint(vendor)) {
+ if (!virStringIsPrintable(vendor)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("disk vendor is not printable string"));
goto error;
@@ -7170,7 +7170,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
- if (!virStrIsPrint(product)) {
+ if (!virStringIsPrintable(product)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("disk product is not printable string"));
goto error;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 068bc00..a79d85e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2265,6 +2265,7 @@ virStringFreeListCount;
virStringGetFirstWithPrefix;
virStringHasControlChars;
virStringIsEmpty;
+virStringIsPrintable;
virStringJoin;
virStringListLength;
virStringReplace;
@@ -2478,7 +2479,6 @@ virSetNonBlock;
virSetSockReuseAddr;
virSetUIDGID;
virSetUIDGIDWithCaps;
-virStrIsPrint;
virTristateBoolTypeFromString;
virTristateBoolTypeToString;
virTristateSwitchTypeFromString;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 2d7fbf3..384e3f7 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1048,3 +1048,21 @@ virStringToUpper(char **dst, const char *src)
*dst = cap;
return 1;
}
+
+
+/**
+ * virStrIsPrintable:
+ *
+ * Returns true @str contains only printable characters.
+ */
+bool
+virStringIsPrintable(const char *str)
+{
+ size_t i;
+
+ for (i = 0; str[i]; i++)
+ if (!c_isprint(str[i]))
+ return false;
+
+ return true;
+}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 16ed3b2..fd2868a 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -275,4 +275,6 @@ void virStringStripIPv6Brackets(char *str);
bool virStringHasControlChars(const char *str);
void virStringStripControlChars(char *str);
+bool virStringIsPrintable(const char *str);
+
#endif /* __VIR_STRING_H__ */
diff --git a/src/util/virutil.c b/src/util/virutil.c
index b401f8d..1b46ea1 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1600,18 +1600,6 @@ virValidateWWN(const char *wwn)
return true;
}
-bool
-virStrIsPrint(const char *str)
-{
- size_t i;
-
- for (i = 0; str[i]; i++)
- if (!c_isprint(str[i]))
- return false;
-
- return true;
-}
-
#if defined(major) && defined(minor)
int
virGetDeviceID(const char *path, int *maj, int *min)
diff --git a/src/util/virutil.h b/src/util/virutil.h
index b121de0..1e51a25 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -152,8 +152,6 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1);
bool virValidateWWN(const char *wwn);
-bool virStrIsPrint(const char *str);
-
int virGetDeviceID(const char *path,
int *maj,
int *min);
--
2.8.0
9:07 a.m.
New subject: [libvirt] [PATCH 01/10] util: Rename and move virStrIsPrint to virStringIsPrintable
On 04/12/2016 09:55 AM, Peter Krempa wrote:
---
src/conf/domain_conf.c | 4 ++--
src/libvirt_private.syms | 2 +-
src/util/virstring.c | 18 ++++++++++++++++++
src/util/virstring.h | 2 ++
src/util/virutil.c | 12 ------------
src/util/virutil.h | 2 --
6 files changed, 23 insertions(+), 17 deletions(-)
ACK
- Cole
1:56 p.m.
New subject: [libvirt] [PATCH 01/10] util: Rename and move virStrIsPrint to virStringIsPrintable
On Tue, Apr 12, 2016 at 15:55:35 +0200, Peter Krempa wrote:
---
src/conf/domain_conf.c | 4 ++--
src/libvirt_private.syms | 2 +-
src/util/virstring.c | 18 ++++++++++++++++++
src/util/virstring.h | 2 ++
src/util/virutil.c | 12 ------------
src/util/virutil.h | 2 --
6 files changed, 23 insertions(+), 17 deletions(-)
...
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 2d7fbf3..384e3f7 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1048,3 +1048,21 @@ virStringToUpper(char **dst, const char *src)
*dst = cap;
return 1;
}
+
+
+/**
+ * virStrIsPrintable:
virStringIsPrintable
Jirka
8:55 a.m.
New subject: [libvirt] [PATCH 02/10] conf: disk: Don't initialize fields allocated by calloc
All the fields were initialized to 0.
---
src/conf/domain_conf.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0320691..aa96bac 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6737,14 +6737,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
if (!(def = virDomainDiskDefNew(xmlopt)))
return NULL;
- def->geometry.cylinders = 0;
- def->geometry.heads = 0;
- def->geometry.sectors = 0;
- def->geometry.trans = VIR_DOMAIN_DISK_TRANS_DEFAULT;
-
- def->blockio.logical_block_size = 0;
- def->blockio.physical_block_size = 0;
-
ctxt->node = node;
type = virXMLPropString(node, "type");
--
2.8.0
9:14 a.m.
New subject: [libvirt] [PATCH 02/10] conf: disk: Don't initialize fields allocated by calloc
On 04/12/2016 09:55 AM, Peter Krempa wrote:
All the fields were initialized to 0.
---
src/conf/domain_conf.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0320691..aa96bac 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6737,14 +6737,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
if (!(def = virDomainDiskDefNew(xmlopt)))
return NULL;
- def->geometry.cylinders = 0;
- def->geometry.heads = 0;
- def->geometry.sectors = 0;
- def->geometry.trans = VIR_DOMAIN_DISK_TRANS_DEFAULT;
-
- def->blockio.logical_block_size = 0;
- def->blockio.physical_block_size = 0;
-
ctxt->node = node;
type = virXMLPropString(node, "type");
ACK
- Cole
8:55 a.m.
New subject: [libvirt] [PATCH 03/10] conf: disk: Remove one unnecessary level of indentation
Also simplify the code by switching to a for loop.
---
src/conf/domain_conf.c | 737 ++++++++++++++++++++++++-------------------------
1 file changed, 368 insertions(+), 369 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aa96bac..f359e95 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6755,423 +6755,422 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
rawio = virXMLPropString(node, "rawio");
sgio = virXMLPropString(node, "sgio");
- cur = node->children;
- while (cur != NULL) {
- if (cur->type == XML_ELEMENT_NODE) {
- if (!source && xmlStrEqual(cur->name, BAD_CAST
"source")) {
- sourceNode = cur;
+ for (cur = node->children; cur != NULL; cur = cur->next) {
+ if (cur->type != XML_ELEMENT_NODE)
+ continue;
- if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0)
- goto error;
+ if (!source && xmlStrEqual(cur->name, BAD_CAST "source")) {
+ sourceNode = cur;
- source = true;
+ if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0)
+ goto error;
- if (def->src->type == VIR_STORAGE_TYPE_NETWORK) {
- if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI)
- expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI;
- else if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
- expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH;
- }
+ source = true;
- startupPolicy = virXMLPropString(cur, "startupPolicy");
+ if (def->src->type == VIR_STORAGE_TYPE_NETWORK) {
+ if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI)
+ expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI;
+ else if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
+ expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH;
+ }
- } else if (!target &&
- xmlStrEqual(cur->name, BAD_CAST "target")) {
- target = virXMLPropString(cur, "dev");
- bus = virXMLPropString(cur, "bus");
- tray = virXMLPropString(cur, "tray");
- removable = virXMLPropString(cur, "removable");
-
- /* HACK: Work around for compat with Xen
- * driver in previous libvirt releases */
- if (target &&
- STRPREFIX(target, "ioemu:"))
- memmove(target, target+6, strlen(target)-5);
- } else if (!domain_name &&
- xmlStrEqual(cur->name, BAD_CAST "backenddomain")) {
- domain_name = virXMLPropString(cur, "name");
- } else if (xmlStrEqual(cur->name, BAD_CAST "geometry")) {
- if (virXPathUInt("string(./geometry/@cyls)",
- ctxt, &def->geometry.cylinders) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("invalid geometry settings (cyls)"));
- goto error;
- }
- if (virXPathUInt("string(./geometry/@heads)",
- ctxt, &def->geometry.heads) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("invalid geometry settings (heads)"));
- goto error;
- }
- if (virXPathUInt("string(./geometry/@secs)",
- ctxt, &def->geometry.sectors) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("invalid geometry settings (secs)"));
- goto error;
- }
- trans = virXMLPropString(cur, "trans");
- if (trans) {
- def->geometry.trans =
virDomainDiskGeometryTransTypeFromString(trans);
- if (def->geometry.trans <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("invalid translation value
'%s'"),
- trans);
- goto error;
- }
- }
- } else if (xmlStrEqual(cur->name, BAD_CAST "blockio")) {
- logical_block_size =
- virXMLPropString(cur, "logical_block_size");
- if (logical_block_size &&
- virStrToLong_ui(logical_block_size, NULL, 0,
- &def->blockio.logical_block_size) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("invalid logical block size
'%s'"),
- logical_block_size);
- goto error;
- }
- physical_block_size =
- virXMLPropString(cur, "physical_block_size");
- if (physical_block_size &&
- virStrToLong_ui(physical_block_size, NULL, 0,
- &def->blockio.physical_block_size) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("invalid physical block size
'%s'"),
- physical_block_size);
+ startupPolicy = virXMLPropString(cur, "startupPolicy");
+
+ } else if (!target &&
+ xmlStrEqual(cur->name, BAD_CAST "target")) {
+ target = virXMLPropString(cur, "dev");
+ bus = virXMLPropString(cur, "bus");
+ tray = virXMLPropString(cur, "tray");
+ removable = virXMLPropString(cur, "removable");
+
+ /* HACK: Work around for compat with Xen
+ * driver in previous libvirt releases */
+ if (target &&
+ STRPREFIX(target, "ioemu:"))
+ memmove(target, target+6, strlen(target)-5);
+ } else if (!domain_name &&
+ xmlStrEqual(cur->name, BAD_CAST "backenddomain")) {
+ domain_name = virXMLPropString(cur, "name");
+ } else if (xmlStrEqual(cur->name, BAD_CAST "geometry")) {
+ if (virXPathUInt("string(./geometry/@cyls)",
+ ctxt, &def->geometry.cylinders) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid geometry settings (cyls)"));
+ goto error;
+ }
+ if (virXPathUInt("string(./geometry/@heads)",
+ ctxt, &def->geometry.heads) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid geometry settings (heads)"));
+ goto error;
+ }
+ if (virXPathUInt("string(./geometry/@secs)",
+ ctxt, &def->geometry.sectors) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid geometry settings (secs)"));
+ goto error;
+ }
+ trans = virXMLPropString(cur, "trans");
+ if (trans) {
+ def->geometry.trans =
virDomainDiskGeometryTransTypeFromString(trans);
+ if (def->geometry.trans <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("invalid translation value
'%s'"),
+ trans);
goto error;
}
- } else if (!driverName &&
- xmlStrEqual(cur->name, BAD_CAST "driver")) {
- driverName = virXMLPropString(cur, "name");
- driverType = virXMLPropString(cur, "type");
- if (STREQ_NULLABLE(driverType, "aio")) {
- /* In-place conversion to "raw", for Xen back-compat */
- driverType[0] = 'r';
- driverType[1] = 'a';
- driverType[2] = 'w';
- }
- cachetag = virXMLPropString(cur, "cache");
- error_policy = virXMLPropString(cur, "error_policy");
- rerror_policy = virXMLPropString(cur, "rerror_policy");
- iotag = virXMLPropString(cur, "io");
- ioeventfd = virXMLPropString(cur, "ioeventfd");
- event_idx = virXMLPropString(cur, "event_idx");
- copy_on_read = virXMLPropString(cur, "copy_on_read");
- discard = virXMLPropString(cur, "discard");
- driverIOThread = virXMLPropString(cur, "iothread");
- } else if (!def->mirror &&
- xmlStrEqual(cur->name, BAD_CAST "mirror") &&
- !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) {
- char *ready;
- char *blockJob;
-
- if (VIR_ALLOC(def->mirror) < 0)
- goto error;
+ }
+ } else if (xmlStrEqual(cur->name, BAD_CAST "blockio")) {
+ logical_block_size =
+ virXMLPropString(cur, "logical_block_size");
+ if (logical_block_size &&
+ virStrToLong_ui(logical_block_size, NULL, 0,
+ &def->blockio.logical_block_size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid logical block size '%s'"),
+ logical_block_size);
+ goto error;
+ }
+ physical_block_size =
+ virXMLPropString(cur, "physical_block_size");
+ if (physical_block_size &&
+ virStrToLong_ui(physical_block_size, NULL, 0,
+ &def->blockio.physical_block_size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid physical block size '%s'"),
+ physical_block_size);
+ goto error;
+ }
+ } else if (!driverName &&
+ xmlStrEqual(cur->name, BAD_CAST "driver")) {
+ driverName = virXMLPropString(cur, "name");
+ driverType = virXMLPropString(cur, "type");
+ if (STREQ_NULLABLE(driverType, "aio")) {
+ /* In-place conversion to "raw", for Xen back-compat */
+ driverType[0] = 'r';
+ driverType[1] = 'a';
+ driverType[2] = 'w';
+ }
+ cachetag = virXMLPropString(cur, "cache");
+ error_policy = virXMLPropString(cur, "error_policy");
+ rerror_policy = virXMLPropString(cur, "rerror_policy");
+ iotag = virXMLPropString(cur, "io");
+ ioeventfd = virXMLPropString(cur, "ioeventfd");
+ event_idx = virXMLPropString(cur, "event_idx");
+ copy_on_read = virXMLPropString(cur, "copy_on_read");
+ discard = virXMLPropString(cur, "discard");
+ driverIOThread = virXMLPropString(cur, "iothread");
+ } else if (!def->mirror &&
+ xmlStrEqual(cur->name, BAD_CAST "mirror") &&
+ !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) {
+ char *ready;
+ char *blockJob;
+
+ if (VIR_ALLOC(def->mirror) < 0)
+ goto error;
- blockJob = virXMLPropString(cur, "job");
- if (blockJob) {
- def->mirrorJob = virDomainBlockJobTypeFromString(blockJob);
- if (def->mirrorJob <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror job type
'%s'"),
- blockJob);
- VIR_FREE(blockJob);
- goto error;
- }
+ blockJob = virXMLPropString(cur, "job");
+ if (blockJob) {
+ def->mirrorJob = virDomainBlockJobTypeFromString(blockJob);
+ if (def->mirrorJob <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown mirror job type '%s'"),
+ blockJob);
VIR_FREE(blockJob);
- } else {
- def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
- }
-
- mirrorType = virXMLPropString(cur, "type");
- if (mirrorType) {
- def->mirror->type = virStorageTypeFromString(mirrorType);
- if (def->mirror->type <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror backing store "
- "type '%s'"), mirrorType);
- goto error;
- }
- mirrorFormat =
virXPathString("string(./mirror/format/@type)",
- ctxt);
- } else {
- /* For back-compat reasons, we handle a file name
- * encoded as attributes, even though we prefer
- * modern output in the style of backingStore */
- def->mirror->type = VIR_STORAGE_TYPE_FILE;
- def->mirror->path = virXMLPropString(cur, "file");
- if (!def->mirror->path) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("mirror requires file name"));
- goto error;
- }
- if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("mirror without type only supported "
- "by copy job"));
- goto error;
- }
- mirrorFormat = virXMLPropString(cur, "format");
- }
- if (mirrorFormat) {
- def->mirror->format =
- virStorageFileFormatTypeFromString(mirrorFormat);
- if (def->mirror->format <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror format value
'%s'"),
- mirrorFormat);
- goto error;
- }
+ goto error;
}
- if (mirrorType) {
- xmlNodePtr mirrorNode;
+ VIR_FREE(blockJob);
+ } else {
+ def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
+ }
- if (!(mirrorNode = virXPathNode("./mirror/source", ctxt)))
{
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("mirror requires source element"));
- goto error;
- }
- if (virDomainDiskSourceParse(mirrorNode, ctxt,
- def->mirror) < 0)
- goto error;
- }
- ready = virXMLPropString(cur, "ready");
- if (ready) {
- if ((def->mirrorState =
- virDomainDiskMirrorStateTypeFromString(ready)) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("unknown mirror ready state %s"),
- ready);
- VIR_FREE(ready);
- goto error;
- }
- VIR_FREE(ready);
- }
- } else if (!authdef &&
- xmlStrEqual(cur->name, BAD_CAST "auth")) {
- if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
- goto error;
- /* Disk volume types won't have the secrettype filled in until
- * after virStorageTranslateDiskSourcePool is run
- */
- if (def->src->type != VIR_STORAGE_TYPE_VOLUME &&
- (auth_secret_usage =
- virSecretUsageTypeFromString(authdef->secrettype)) < 0) {
+ mirrorType = virXMLPropString(cur, "type");
+ if (mirrorType) {
+ def->mirror->type = virStorageTypeFromString(mirrorType);
+ if (def->mirror->type <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("invalid secret type %s"),
- authdef->secrettype);
+ _("unknown mirror backing store "
+ "type '%s'"), mirrorType);
goto error;
}
- } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) {
- ret = virXPathULongLong("string(./iotune/total_bytes_sec)",
- ctxt,
- &def->blkdeviotune.total_bytes_sec);
- if (ret == -2) {
+ mirrorFormat = virXPathString("string(./mirror/format/@type)",
+ ctxt);
+ } else {
+ /* For back-compat reasons, we handle a file name
+ * encoded as attributes, even though we prefer
+ * modern output in the style of backingStore */
+ def->mirror->type = VIR_STORAGE_TYPE_FILE;
+ def->mirror->path = virXMLPropString(cur, "file");
+ if (!def->mirror->path) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total throughput limit must be an
integer"));
+ _("mirror requires file name"));
goto error;
- } else if (ret < 0) {
- def->blkdeviotune.total_bytes_sec = 0;
}
-
- ret = virXPathULongLong("string(./iotune/read_bytes_sec)",
- ctxt,
- &def->blkdeviotune.read_bytes_sec);
- if (ret == -2) {
+ if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("read throughput limit must be an
integer"));
+ _("mirror without type only supported "
+ "by copy job"));
goto error;
- } else if (ret < 0) {
- def->blkdeviotune.read_bytes_sec = 0;
}
-
- ret = virXPathULongLong("string(./iotune/write_bytes_sec)",
- ctxt,
- &def->blkdeviotune.write_bytes_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("write throughput limit must be an
integer"));
+ mirrorFormat = virXMLPropString(cur, "format");
+ }
+ if (mirrorFormat) {
+ def->mirror->format =
+ virStorageFileFormatTypeFromString(mirrorFormat);
+ if (def->mirror->format <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown mirror format value
'%s'"),
+ mirrorFormat);
goto error;
- } else if (ret < 0) {
- def->blkdeviotune.write_bytes_sec = 0;
}
+ }
+ if (mirrorType) {
+ xmlNodePtr mirrorNode;
- ret = virXPathULongLong("string(./iotune/total_iops_sec)",
- ctxt,
- &def->blkdeviotune.total_iops_sec);
- if (ret == -2) {
+ if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total I/O operations limit must be an
integer"));
+ _("mirror requires source element"));
goto error;
- } else if (ret < 0) {
- def->blkdeviotune.total_iops_sec = 0;
}
-
- ret = virXPathULongLong("string(./iotune/read_iops_sec)",
- ctxt,
- &def->blkdeviotune.read_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("read I/O operations limit must be an
integer"));
+ if (virDomainDiskSourceParse(mirrorNode, ctxt,
+ def->mirror) < 0)
goto error;
- } else if (ret < 0) {
- def->blkdeviotune.read_iops_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/write_iops_sec)",
- ctxt,
- &def->blkdeviotune.write_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("write I/O operations limit must be an
integer"));
+ }
+ ready = virXMLPropString(cur, "ready");
+ if (ready) {
+ if ((def->mirrorState =
+ virDomainDiskMirrorStateTypeFromString(ready)) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unknown mirror ready state %s"),
+ ready);
+ VIR_FREE(ready);
goto error;
- } else if (ret < 0) {
- def->blkdeviotune.write_iops_sec = 0;
}
+ VIR_FREE(ready);
+ }
+ } else if (!authdef &&
+ xmlStrEqual(cur->name, BAD_CAST "auth")) {
+ if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
+ goto error;
+ /* Disk volume types won't have the secrettype filled in until
+ * after virStorageTranslateDiskSourcePool is run
+ */
+ if (def->src->type != VIR_STORAGE_TYPE_VOLUME &&
+ (auth_secret_usage =
+ virSecretUsageTypeFromString(authdef->secrettype)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("invalid secret type %s"),
+ authdef->secrettype);
+ goto error;
+ }
+ } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) {
+ ret = virXPathULongLong("string(./iotune/total_bytes_sec)",
+ ctxt,
+ &def->blkdeviotune.total_bytes_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total throughput limit must be an
integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.total_bytes_sec = 0;
+ }
- if (virXPathULongLong("string(./iotune/total_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.total_bytes_sec_max) <
0) {
- def->blkdeviotune.total_bytes_sec_max = 0;
- }
+ ret = virXPathULongLong("string(./iotune/read_bytes_sec)",
+ ctxt,
+ &def->blkdeviotune.read_bytes_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("read throughput limit must be an integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.read_bytes_sec = 0;
+ }
- if (virXPathULongLong("string(./iotune/read_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.read_bytes_sec_max) <
0) {
- def->blkdeviotune.read_bytes_sec_max = 0;
- }
+ ret = virXPathULongLong("string(./iotune/write_bytes_sec)",
+ ctxt,
+ &def->blkdeviotune.write_bytes_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("write throughput limit must be an
integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.write_bytes_sec = 0;
+ }
- if (virXPathULongLong("string(./iotune/write_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.write_bytes_sec_max) <
0) {
- def->blkdeviotune.write_bytes_sec_max = 0;
- }
+ ret = virXPathULongLong("string(./iotune/total_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.total_iops_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total I/O operations limit must be an
integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.total_iops_sec = 0;
+ }
- if (virXPathULongLong("string(./iotune/total_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.total_iops_sec_max) <
0) {
- def->blkdeviotune.total_iops_sec_max = 0;
- }
+ ret = virXPathULongLong("string(./iotune/read_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.read_iops_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("read I/O operations limit must be an
integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.read_iops_sec = 0;
+ }
- if (virXPathULongLong("string(./iotune/read_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.read_iops_sec_max) <
0) {
- def->blkdeviotune.read_iops_sec_max = 0;
- }
+ ret = virXPathULongLong("string(./iotune/write_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.write_iops_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("write I/O operations limit must be an
integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.write_iops_sec = 0;
+ }
- if (virXPathULongLong("string(./iotune/write_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.write_iops_sec_max) <
0) {
- def->blkdeviotune.write_iops_sec_max = 0;
- }
+ if (virXPathULongLong("string(./iotune/total_bytes_sec_max)",
+ ctxt,
+ &def->blkdeviotune.total_bytes_sec_max) < 0)
{
+ def->blkdeviotune.total_bytes_sec_max = 0;
+ }
- if (virXPathULongLong("string(./iotune/size_iops_sec)",
- ctxt,
- &def->blkdeviotune.size_iops_sec) < 0) {
- def->blkdeviotune.size_iops_sec = 0;
- }
+ if (virXPathULongLong("string(./iotune/read_bytes_sec_max)",
+ ctxt,
+ &def->blkdeviotune.read_bytes_sec_max) < 0)
{
+ def->blkdeviotune.read_bytes_sec_max = 0;
+ }
+ if (virXPathULongLong("string(./iotune/write_bytes_sec_max)",
+ ctxt,
+ &def->blkdeviotune.write_bytes_sec_max) < 0)
{
+ def->blkdeviotune.write_bytes_sec_max = 0;
+ }
- if ((def->blkdeviotune.total_bytes_sec &&
- def->blkdeviotune.read_bytes_sec) ||
- (def->blkdeviotune.total_bytes_sec &&
- def->blkdeviotune.write_bytes_sec)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write bytes_sec "
- "cannot be set at the same time"));
- goto error;
- }
+ if (virXPathULongLong("string(./iotune/total_iops_sec_max)",
+ ctxt,
+ &def->blkdeviotune.total_iops_sec_max) < 0)
{
+ def->blkdeviotune.total_iops_sec_max = 0;
+ }
- if ((def->blkdeviotune.total_iops_sec &&
- def->blkdeviotune.read_iops_sec) ||
- (def->blkdeviotune.total_iops_sec &&
- def->blkdeviotune.write_iops_sec)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write iops_sec "
- "cannot be set at the same time"));
- goto error;
- }
+ if (virXPathULongLong("string(./iotune/read_iops_sec_max)",
+ ctxt,
+ &def->blkdeviotune.read_iops_sec_max) < 0) {
+ def->blkdeviotune.read_iops_sec_max = 0;
+ }
- if ((def->blkdeviotune.total_bytes_sec_max &&
- def->blkdeviotune.read_bytes_sec_max) ||
- (def->blkdeviotune.total_bytes_sec_max &&
- def->blkdeviotune.write_bytes_sec_max)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write bytes_sec_max "
- "cannot be set at the same time"));
- goto error;
- }
+ if (virXPathULongLong("string(./iotune/write_iops_sec_max)",
+ ctxt,
+ &def->blkdeviotune.write_iops_sec_max) < 0)
{
+ def->blkdeviotune.write_iops_sec_max = 0;
+ }
- if ((def->blkdeviotune.total_iops_sec_max &&
- def->blkdeviotune.read_iops_sec_max) ||
- (def->blkdeviotune.total_iops_sec_max &&
- def->blkdeviotune.write_iops_sec_max)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write iops_sec_max "
- "cannot be set at the same time"));
- goto error;
- }
+ if (virXPathULongLong("string(./iotune/size_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.size_iops_sec) < 0) {
+ def->blkdeviotune.size_iops_sec = 0;
+ }
- } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
- def->src->readonly = true;
- } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
- def->src->shared = true;
- } else if (xmlStrEqual(cur->name, BAD_CAST "transient")) {
- def->transient = true;
- } else if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) &&
- xmlStrEqual(cur->name, BAD_CAST "state")) {
- /* Legacy back-compat. Don't add any more attributes here */
- devaddr = virXMLPropString(cur, "devaddr");
- } else if (encryption == NULL &&
- xmlStrEqual(cur->name, BAD_CAST "encryption")) {
- encryption = virStorageEncryptionParseNode(node->doc,
- cur);
- if (encryption == NULL)
- goto error;
- } else if (!serial &&
- xmlStrEqual(cur->name, BAD_CAST "serial")) {
- serial = (char *)xmlNodeGetContent(cur);
- } else if (!wwn &&
- xmlStrEqual(cur->name, BAD_CAST "wwn")) {
- wwn = (char *)xmlNodeGetContent(cur);
-
- if (!virValidateWWN(wwn))
- goto error;
- } else if (!vendor &&
- xmlStrEqual(cur->name, BAD_CAST "vendor")) {
- vendor = (char *)xmlNodeGetContent(cur);
- if (strlen(vendor) > VENDOR_LEN) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk vendor is more than 8
characters"));
- goto error;
- }
+ if ((def->blkdeviotune.total_bytes_sec &&
+ def->blkdeviotune.read_bytes_sec) ||
+ (def->blkdeviotune.total_bytes_sec &&
+ def->blkdeviotune.write_bytes_sec)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write bytes_sec "
+ "cannot be set at the same time"));
+ goto error;
+ }
- if (!virStringIsPrintable(vendor)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk vendor is not printable string"));
- goto error;
- }
- } else if (!product &&
- xmlStrEqual(cur->name, BAD_CAST "product")) {
- product = (char *)xmlNodeGetContent(cur);
+ if ((def->blkdeviotune.total_iops_sec &&
+ def->blkdeviotune.read_iops_sec) ||
+ (def->blkdeviotune.total_iops_sec &&
+ def->blkdeviotune.write_iops_sec)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write iops_sec "
+ "cannot be set at the same time"));
+ goto error;
+ }
- if (strlen(product) > PRODUCT_LEN) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk product is more than 16
characters"));
- goto error;
- }
+ if ((def->blkdeviotune.total_bytes_sec_max &&
+ def->blkdeviotune.read_bytes_sec_max) ||
+ (def->blkdeviotune.total_bytes_sec_max &&
+ def->blkdeviotune.write_bytes_sec_max)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write bytes_sec_max "
+ "cannot be set at the same time"));
+ goto error;
+ }
- if (!virStringIsPrintable(product)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk product is not printable string"));
- goto error;
- }
- } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) {
- /* boot is parsed as part of virDomainDeviceInfoParseXML */
+ if ((def->blkdeviotune.total_iops_sec_max &&
+ def->blkdeviotune.read_iops_sec_max) ||
+ (def->blkdeviotune.total_iops_sec_max &&
+ def->blkdeviotune.write_iops_sec_max)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write iops_sec_max "
+ "cannot be set at the same time"));
+ goto error;
+ }
+
+ } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
+ def->src->readonly = true;
+ } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
+ def->src->shared = true;
+ } else if (xmlStrEqual(cur->name, BAD_CAST "transient")) {
+ def->transient = true;
+ } else if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) &&
+ xmlStrEqual(cur->name, BAD_CAST "state")) {
+ /* Legacy back-compat. Don't add any more attributes here */
+ devaddr = virXMLPropString(cur, "devaddr");
+ } else if (encryption == NULL &&
+ xmlStrEqual(cur->name, BAD_CAST "encryption")) {
+ encryption = virStorageEncryptionParseNode(node->doc,
+ cur);
+ if (encryption == NULL)
+ goto error;
+ } else if (!serial &&
+ xmlStrEqual(cur->name, BAD_CAST "serial")) {
+ serial = (char *)xmlNodeGetContent(cur);
+ } else if (!wwn &&
+ xmlStrEqual(cur->name, BAD_CAST "wwn")) {
+ wwn = (char *)xmlNodeGetContent(cur);
+
+ if (!virValidateWWN(wwn))
+ goto error;
+ } else if (!vendor &&
+ xmlStrEqual(cur->name, BAD_CAST "vendor")) {
+ vendor = (char *)xmlNodeGetContent(cur);
+
+ if (strlen(vendor) > VENDOR_LEN) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("disk vendor is more than 8 characters"));
+ goto error;
}
+
+ if (!virStringIsPrintable(vendor)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("disk vendor is not printable string"));
+ goto error;
+ }
+ } else if (!product &&
+ xmlStrEqual(cur->name, BAD_CAST "product")) {
+ product = (char *)xmlNodeGetContent(cur);
+
+ if (strlen(product) > PRODUCT_LEN) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("disk product is more than 16 characters"));
+ goto error;
+ }
+
+ if (!virStringIsPrintable(product)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("disk product is not printable string"));
+ goto error;
+ }
+ } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) {
+ /* boot is parsed as part of virDomainDeviceInfoParseXML */
}
- cur = cur->next;
}
/* Disk volume types will have authentication information handled in
--
2.8.0
9:15 a.m.
New subject: [libvirt] [PATCH 03/10] conf: disk: Remove one unnecessary level of indentation
On 04/12/2016 09:55 AM, Peter Krempa wrote:
Also simplify the code by switching to a for loop.
---
src/conf/domain_conf.c | 737 ++++++++++++++++++++++++-------------------------
1 file changed, 368 insertions(+), 369 deletions(-)
git show -w makes it clear:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fb5d327..f691174 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6760,9 +6760,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
rawio = virXMLPropString(node, "rawio");
sgio = virXMLPropString(node, "sgio");
- cur = node->children;
- while (cur != NULL) {
- if (cur->type == XML_ELEMENT_NODE) {
+ for (cur = node->children; cur != NULL; cur = cur->next) {
+ if (cur->type != XML_ELEMENT_NODE)
+ continue;
+
if (!source && xmlStrEqual(cur->name, BAD_CAST "source")) {
sourceNode = cur;
@@ -7176,8 +7177,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
/* boot is parsed as part of virDomainDeviceInfoParseXML */
}
}
- cur = cur->next;
- }
/* Disk volume types will have authentication information handled in
* virStorageTranslateDiskSourcePool
ACK
- Cole
8:55 a.m.
New subject: [libvirt] [PATCH 04/10] conf: disk: Extract iotune parsing into a separate func
---
src/conf/domain_conf.c | 312 ++++++++++++++++++++++++++-----------------------
1 file changed, 163 insertions(+), 149 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f359e95..c2ac8d6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6676,6 +6676,168 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
}
+static int
+virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
+ xmlXPathContextPtr ctxt)
+{
+ int ret;
+
+ ret = virXPathULongLong("string(./iotune/total_bytes_sec)",
+ ctxt,
+ &def->blkdeviotune.total_bytes_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total throughput limit must be an integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.total_bytes_sec = 0;
+ }
+
+ ret = virXPathULongLong("string(./iotune/read_bytes_sec)",
+ ctxt,
+ &def->blkdeviotune.read_bytes_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("read throughput limit must be an integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.read_bytes_sec = 0;
+ }
+
+ ret = virXPathULongLong("string(./iotune/write_bytes_sec)",
+ ctxt,
+ &def->blkdeviotune.write_bytes_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("write throughput limit must be an integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.write_bytes_sec = 0;
+ }
+
+ ret = virXPathULongLong("string(./iotune/total_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.total_iops_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total I/O operations limit must be an integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.total_iops_sec = 0;
+ }
+
+ ret = virXPathULongLong("string(./iotune/read_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.read_iops_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("read I/O operations limit must be an integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.read_iops_sec = 0;
+ }
+
+ ret = virXPathULongLong("string(./iotune/write_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.write_iops_sec);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("write I/O operations limit must be an integer"));
+ goto error;
+ } else if (ret < 0) {
+ def->blkdeviotune.write_iops_sec = 0;
+ }
+
+ if (virXPathULongLong("string(./iotune/total_bytes_sec_max)",
+ ctxt,
+ &def->blkdeviotune.total_bytes_sec_max) < 0) {
+ def->blkdeviotune.total_bytes_sec_max = 0;
+ }
+
+ if (virXPathULongLong("string(./iotune/read_bytes_sec_max)",
+ ctxt,
+ &def->blkdeviotune.read_bytes_sec_max) < 0) {
+ def->blkdeviotune.read_bytes_sec_max = 0;
+ }
+
+ if (virXPathULongLong("string(./iotune/write_bytes_sec_max)",
+ ctxt,
+ &def->blkdeviotune.write_bytes_sec_max) < 0) {
+ def->blkdeviotune.write_bytes_sec_max = 0;
+ }
+
+ if (virXPathULongLong("string(./iotune/total_iops_sec_max)",
+ ctxt,
+ &def->blkdeviotune.total_iops_sec_max) < 0) {
+ def->blkdeviotune.total_iops_sec_max = 0;
+ }
+
+ if (virXPathULongLong("string(./iotune/read_iops_sec_max)",
+ ctxt,
+ &def->blkdeviotune.read_iops_sec_max) < 0) {
+ def->blkdeviotune.read_iops_sec_max = 0;
+ }
+
+ if (virXPathULongLong("string(./iotune/write_iops_sec_max)",
+ ctxt,
+ &def->blkdeviotune.write_iops_sec_max) < 0) {
+ def->blkdeviotune.write_iops_sec_max = 0;
+ }
+
+ if (virXPathULongLong("string(./iotune/size_iops_sec)",
+ ctxt,
+ &def->blkdeviotune.size_iops_sec) < 0) {
+ def->blkdeviotune.size_iops_sec = 0;
+ }
+
+
+ if ((def->blkdeviotune.total_bytes_sec &&
+ def->blkdeviotune.read_bytes_sec) ||
+ (def->blkdeviotune.total_bytes_sec &&
+ def->blkdeviotune.write_bytes_sec)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write bytes_sec "
+ "cannot be set at the same time"));
+ goto error;
+ }
+
+ if ((def->blkdeviotune.total_iops_sec &&
+ def->blkdeviotune.read_iops_sec) ||
+ (def->blkdeviotune.total_iops_sec &&
+ def->blkdeviotune.write_iops_sec)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write iops_sec "
+ "cannot be set at the same time"));
+ goto error;
+ }
+
+ if ((def->blkdeviotune.total_bytes_sec_max &&
+ def->blkdeviotune.read_bytes_sec_max) ||
+ (def->blkdeviotune.total_bytes_sec_max &&
+ def->blkdeviotune.write_bytes_sec_max)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write bytes_sec_max "
+ "cannot be set at the same time"));
+ goto error;
+ }
+
+ if ((def->blkdeviotune.total_iops_sec_max &&
+ def->blkdeviotune.read_iops_sec_max) ||
+ (def->blkdeviotune.total_iops_sec_max &&
+ def->blkdeviotune.write_iops_sec_max)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write iops_sec_max "
+ "cannot be set at the same time"));
+ goto error;
+ }
+
+ return 0;
+
+ error:
+ return ret;
+}
+
+
#define VENDOR_LEN 8
#define PRODUCT_LEN 16
@@ -6732,7 +6894,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
char *domain_name = NULL;
int expected_secret_usage = -1;
int auth_secret_usage = -1;
- int ret = 0;
if (!(def = virDomainDiskDefNew(xmlopt)))
return NULL;
@@ -6964,155 +7125,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
} else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) {
- ret = virXPathULongLong("string(./iotune/total_bytes_sec)",
- ctxt,
- &def->blkdeviotune.total_bytes_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total throughput limit must be an
integer"));
- goto error;
- } else if (ret < 0) {
- def->blkdeviotune.total_bytes_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/read_bytes_sec)",
- ctxt,
- &def->blkdeviotune.read_bytes_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("read throughput limit must be an integer"));
- goto error;
- } else if (ret < 0) {
- def->blkdeviotune.read_bytes_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/write_bytes_sec)",
- ctxt,
- &def->blkdeviotune.write_bytes_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("write throughput limit must be an
integer"));
- goto error;
- } else if (ret < 0) {
- def->blkdeviotune.write_bytes_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/total_iops_sec)",
- ctxt,
- &def->blkdeviotune.total_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total I/O operations limit must be an
integer"));
- goto error;
- } else if (ret < 0) {
- def->blkdeviotune.total_iops_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/read_iops_sec)",
- ctxt,
- &def->blkdeviotune.read_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("read I/O operations limit must be an
integer"));
- goto error;
- } else if (ret < 0) {
- def->blkdeviotune.read_iops_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/write_iops_sec)",
- ctxt,
- &def->blkdeviotune.write_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("write I/O operations limit must be an
integer"));
+ if (virDomainDiskDefIotuneParse(def, ctxt) < 0)
goto error;
- } else if (ret < 0) {
- def->blkdeviotune.write_iops_sec = 0;
- }
-
- if (virXPathULongLong("string(./iotune/total_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.total_bytes_sec_max) < 0)
{
- def->blkdeviotune.total_bytes_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/read_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.read_bytes_sec_max) < 0)
{
- def->blkdeviotune.read_bytes_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/write_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.write_bytes_sec_max) < 0)
{
- def->blkdeviotune.write_bytes_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/total_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.total_iops_sec_max) < 0)
{
- def->blkdeviotune.total_iops_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/read_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.read_iops_sec_max) < 0) {
- def->blkdeviotune.read_iops_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/write_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.write_iops_sec_max) < 0)
{
- def->blkdeviotune.write_iops_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/size_iops_sec)",
- ctxt,
- &def->blkdeviotune.size_iops_sec) < 0) {
- def->blkdeviotune.size_iops_sec = 0;
- }
-
-
- if ((def->blkdeviotune.total_bytes_sec &&
- def->blkdeviotune.read_bytes_sec) ||
- (def->blkdeviotune.total_bytes_sec &&
- def->blkdeviotune.write_bytes_sec)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write bytes_sec "
- "cannot be set at the same time"));
- goto error;
- }
-
- if ((def->blkdeviotune.total_iops_sec &&
- def->blkdeviotune.read_iops_sec) ||
- (def->blkdeviotune.total_iops_sec &&
- def->blkdeviotune.write_iops_sec)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write iops_sec "
- "cannot be set at the same time"));
- goto error;
- }
-
- if ((def->blkdeviotune.total_bytes_sec_max &&
- def->blkdeviotune.read_bytes_sec_max) ||
- (def->blkdeviotune.total_bytes_sec_max &&
- def->blkdeviotune.write_bytes_sec_max)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write bytes_sec_max "
- "cannot be set at the same time"));
- goto error;
- }
-
- if ((def->blkdeviotune.total_iops_sec_max &&
- def->blkdeviotune.read_iops_sec_max) ||
- (def->blkdeviotune.total_iops_sec_max &&
- def->blkdeviotune.write_iops_sec_max)) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write iops_sec_max "
- "cannot be set at the same time"));
- goto error;
- }
-
} else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
def->src->readonly = true;
} else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
--
2.8.0
9:20 a.m.
New subject: [libvirt] [PATCH 04/10] conf: disk: Extract iotune parsing into a separate func
On 04/12/2016 09:55 AM, Peter Krempa wrote:
---
src/conf/domain_conf.c | 312 ++++++++++++++++++++++++++-----------------------
1 file changed, 163 insertions(+), 149 deletions(-)
ACK
- Cole
8:55 a.m.
New subject: [libvirt] [PATCH 05/10] conf: disk: Remove error label from virDomainDiskDefIotuneParse
Since this function isn't doing any cleanup, the label is not necessary.
---
src/conf/domain_conf.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c2ac8d6..ac464d3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6688,7 +6688,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
if (ret == -2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("total throughput limit must be an integer"));
- goto error;
+ return -1;
} else if (ret < 0) {
def->blkdeviotune.total_bytes_sec = 0;
}
@@ -6699,7 +6699,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
if (ret == -2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("read throughput limit must be an integer"));
- goto error;
+ return -1;
} else if (ret < 0) {
def->blkdeviotune.read_bytes_sec = 0;
}
@@ -6710,7 +6710,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
if (ret == -2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("write throughput limit must be an integer"));
- goto error;
+ return -1;
} else if (ret < 0) {
def->blkdeviotune.write_bytes_sec = 0;
}
@@ -6721,7 +6721,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
if (ret == -2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("total I/O operations limit must be an integer"));
- goto error;
+ return -1;
} else if (ret < 0) {
def->blkdeviotune.total_iops_sec = 0;
}
@@ -6732,7 +6732,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
if (ret == -2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("read I/O operations limit must be an integer"));
- goto error;
+ return -1;
} else if (ret < 0) {
def->blkdeviotune.read_iops_sec = 0;
}
@@ -6743,7 +6743,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
if (ret == -2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("write I/O operations limit must be an integer"));
- goto error;
+ return -1;
} else if (ret < 0) {
def->blkdeviotune.write_iops_sec = 0;
}
@@ -6798,7 +6798,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
virReportError(VIR_ERR_XML_ERROR, "%s",
_("total and read/write bytes_sec "
"cannot be set at the same time"));
- goto error;
+ return -1;
}
if ((def->blkdeviotune.total_iops_sec &&
@@ -6808,7 +6808,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
virReportError(VIR_ERR_XML_ERROR, "%s",
_("total and read/write iops_sec "
"cannot be set at the same time"));
- goto error;
+ return -1;
}
if ((def->blkdeviotune.total_bytes_sec_max &&
@@ -6818,7 +6818,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
virReportError(VIR_ERR_XML_ERROR, "%s",
_("total and read/write bytes_sec_max "
"cannot be set at the same time"));
- goto error;
+ return -1;
}
if ((def->blkdeviotune.total_iops_sec_max &&
@@ -6828,13 +6828,10 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
virReportError(VIR_ERR_XML_ERROR, "%s",
_("total and read/write iops_sec_max "
"cannot be set at the same time"));
- goto error;
+ return -1;
}
return 0;
-
- error:
- return ret;
}
--
2.8.0
9:22 a.m.
New subject: [libvirt] [PATCH 05/10] conf: disk: Remove error label from virDomainDiskDefIotuneParse
On 04/12/2016 09:55 AM, Peter Krempa wrote:
Since this function isn't doing any cleanup, the label is not
necessary.
---
src/conf/domain_conf.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
ACK
- Cole
8:55 a.m.
New subject: [libvirt] [PATCH 06/10] conf: virDomainDiskDefIotuneParse: simplfiy parsing
Since the structure was pre-initialized to 0 we don't need to set every
single member to 0 if it's not present in the XML. Additionally if we
put the name of the field into the error message the code can be
simplified using a macro to parse the members.
---
src/conf/domain_conf.c | 81 +++++++++-----------------------------------------
1 file changed, 14 insertions(+), 67 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ac464d3..3c066b4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6675,78 +6675,24 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
return ret;
}
+#define PARSE_IOTUNE(val) \
+ if (virXPathULongLong("string(./iotune/" #val ")",
\
+ ctxt, &def->blkdeviotune.val) == -2) { \
+ virReportError(VIR_ERR_XML_ERROR, \
+ _("disk iotune field '%s' must be an integer"),
#val); \
+ return -1; \
+ }
static int
virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
xmlXPathContextPtr ctxt)
{
- int ret;
-
- ret = virXPathULongLong("string(./iotune/total_bytes_sec)",
- ctxt,
- &def->blkdeviotune.total_bytes_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total throughput limit must be an integer"));
- return -1;
- } else if (ret < 0) {
- def->blkdeviotune.total_bytes_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/read_bytes_sec)",
- ctxt,
- &def->blkdeviotune.read_bytes_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("read throughput limit must be an integer"));
- return -1;
- } else if (ret < 0) {
- def->blkdeviotune.read_bytes_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/write_bytes_sec)",
- ctxt,
- &def->blkdeviotune.write_bytes_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("write throughput limit must be an integer"));
- return -1;
- } else if (ret < 0) {
- def->blkdeviotune.write_bytes_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/total_iops_sec)",
- ctxt,
- &def->blkdeviotune.total_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total I/O operations limit must be an integer"));
- return -1;
- } else if (ret < 0) {
- def->blkdeviotune.total_iops_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/read_iops_sec)",
- ctxt,
- &def->blkdeviotune.read_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("read I/O operations limit must be an integer"));
- return -1;
- } else if (ret < 0) {
- def->blkdeviotune.read_iops_sec = 0;
- }
-
- ret = virXPathULongLong("string(./iotune/write_iops_sec)",
- ctxt,
- &def->blkdeviotune.write_iops_sec);
- if (ret == -2) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("write I/O operations limit must be an integer"));
- return -1;
- } else if (ret < 0) {
- def->blkdeviotune.write_iops_sec = 0;
- }
+ PARSE_IOTUNE(total_bytes_sec);
+ PARSE_IOTUNE(read_bytes_sec);
+ PARSE_IOTUNE(write_bytes_sec);
+ PARSE_IOTUNE(total_iops_sec);
+ PARSE_IOTUNE(read_iops_sec);
+ PARSE_IOTUNE(write_iops_sec);
if (virXPathULongLong("string(./iotune/total_bytes_sec_max)",
ctxt,
@@ -6833,6 +6779,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
return 0;
}
+#undef PARSE_IOTUNE
#define VENDOR_LEN 8
--
2.8.0
9:29 a.m.
New subject: [libvirt] [PATCH 06/10] conf: virDomainDiskDefIotuneParse: simplfiy parsing
On 04/12/2016 09:55 AM, Peter Krempa wrote:
Since the structure was pre-initialized to 0 we don't need to set
every
single member to 0 if it's not present in the XML. Additionally if we
put the name of the field into the error message the code can be
simplified using a macro to parse the members.
---
src/conf/domain_conf.c | 81 +++++++++-----------------------------------------
1 file changed, 14 insertions(+), 67 deletions(-)
Technically virXPathULongLong can thrown an error with -1 if one of the passed
values is NULL, but old code wasn't handling that, and virxml.c should
probably either drop those conditionals or use a separate error code for it.
ACK
- Cole
9:37 a.m.
New subject: [libvirt] [PATCH 06/10] conf: virDomainDiskDefIotuneParse: simplfiy parsing
s/simplfiy/simplify/ in the commit summary.
Jan
8:55 a.m.
New subject: [libvirt] [PATCH 07/10] conf: virDomainDiskDefIotuneParse: Report malformed number errors
Rest of the fields of the iotune data structure did not check for
malformed integers. Use the previously defined macro to extract them
which will simplify the code and add error reporting.
---
src/conf/domain_conf.c | 50 ++++++++------------------------------------------
1 file changed, 8 insertions(+), 42 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3c066b4..64561df 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6694,48 +6694,14 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
PARSE_IOTUNE(read_iops_sec);
PARSE_IOTUNE(write_iops_sec);
- if (virXPathULongLong("string(./iotune/total_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.total_bytes_sec_max) < 0) {
- def->blkdeviotune.total_bytes_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/read_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.read_bytes_sec_max) < 0) {
- def->blkdeviotune.read_bytes_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/write_bytes_sec_max)",
- ctxt,
- &def->blkdeviotune.write_bytes_sec_max) < 0) {
- def->blkdeviotune.write_bytes_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/total_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.total_iops_sec_max) < 0) {
- def->blkdeviotune.total_iops_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/read_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.read_iops_sec_max) < 0) {
- def->blkdeviotune.read_iops_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/write_iops_sec_max)",
- ctxt,
- &def->blkdeviotune.write_iops_sec_max) < 0) {
- def->blkdeviotune.write_iops_sec_max = 0;
- }
-
- if (virXPathULongLong("string(./iotune/size_iops_sec)",
- ctxt,
- &def->blkdeviotune.size_iops_sec) < 0) {
- def->blkdeviotune.size_iops_sec = 0;
- }
-
+ PARSE_IOTUNE(total_bytes_sec_max);
+ PARSE_IOTUNE(read_bytes_sec_max);
+ PARSE_IOTUNE(write_bytes_sec_max);
+ PARSE_IOTUNE(total_iops_sec_max);
+ PARSE_IOTUNE(read_iops_sec_max);
+ PARSE_IOTUNE(write_iops_sec_max);
+
+ PARSE_IOTUNE(size_iops_sec);
if ((def->blkdeviotune.total_bytes_sec &&
def->blkdeviotune.read_bytes_sec) ||
--
2.8.0
9:31 a.m.
New subject: [libvirt] [PATCH 07/10] conf: virDomainDiskDefIotuneParse: Report malformed number errors
On 04/12/2016 09:55 AM, Peter Krempa wrote:
Rest of the fields of the iotune data structure did not check for
malformed integers. Use the previously defined macro to extract them
which will simplify the code and add error reporting.
---
src/conf/domain_conf.c | 50 ++++++++------------------------------------------
1 file changed, 8 insertions(+), 42 deletions(-)
ACK... we need more of these cleanups, there's way too much boilerplate in the
codebase (he says as he just tried to add an extra 100 with setlocale error
checking :) )
- Cole
1:24 a.m.
New subject: [libvirt] [PATCH 07/10] conf: virDomainDiskDefIotuneParse: Report malformed number errors
On Tue, Apr 12, 2016 at 10:31:29 -0400, Cole Robinson wrote:
On 04/12/2016 09:55 AM, Peter Krempa wrote:
> Rest of the fields of the iotune data structure did not check for
> malformed integers. Use the previously defined macro to extract them
> which will simplify the code and add error reporting.
> ---
> src/conf/domain_conf.c | 50 ++++++++------------------------------------------
> 1 file changed, 8 insertions(+), 42 deletions(-)
>
ACK... we need more of these cleanups, there's way too much boilerplate in the
codebase (he says as he just tried to add an extra 100 with setlocale error
checking :) )
Adding new code is inevitable. I find more troubling that we have
functions spanning thousands of lines and we are even adding code to
them from time to time. Spaghetti code must die.
I try to keep the functions so that they fit on one screen, but that
measure still isn't entirely universal since I use vertical screens :D.
Peter
8:55 a.m.
New subject: [libvirt] [PATCH 08/10] conf: disk: Split out parsing of disk mirror data
Changes are indentation and 'cleanup' label instead of 'error'.
---
src/conf/domain_conf.c | 192 ++++++++++++++++++++++++++-----------------------
1 file changed, 104 insertions(+), 88 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 64561df..802a6fe 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6748,6 +6748,109 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
#undef PARSE_IOTUNE
+static int
+virDomainDiskDefMirrorParse(virDomainDiskDefPtr def,
+ xmlNodePtr cur,
+ xmlXPathContextPtr ctxt)
+{
+ char *mirrorFormat = NULL;
+ char *mirrorType = NULL;
+ char *ready;
+ char *blockJob;
+ int ret = -1;
+
+ if (VIR_ALLOC(def->mirror) < 0)
+ goto cleanup;
+
+ blockJob = virXMLPropString(cur, "job");
+ if (blockJob) {
+ def->mirrorJob = virDomainBlockJobTypeFromString(blockJob);
+ if (def->mirrorJob <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown mirror job type '%s'"),
+ blockJob);
+ VIR_FREE(blockJob);
+ goto cleanup;
+ }
+ VIR_FREE(blockJob);
+ } else {
+ def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
+ }
+
+ mirrorType = virXMLPropString(cur, "type");
+ if (mirrorType) {
+ def->mirror->type = virStorageTypeFromString(mirrorType);
+ if (def->mirror->type <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown mirror backing store "
+ "type '%s'"), mirrorType);
+ goto cleanup;
+ }
+ mirrorFormat = virXPathString("string(./mirror/format/@type)",
+ ctxt);
+ } else {
+ /* For back-compat reasons, we handle a file name
+ * encoded as attributes, even though we prefer
+ * modern output in the style of backingStore */
+ def->mirror->type = VIR_STORAGE_TYPE_FILE;
+ def->mirror->path = virXMLPropString(cur, "file");
+ if (!def->mirror->path) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("mirror requires file name"));
+ goto cleanup;
+ }
+ if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("mirror without type only supported "
+ "by copy job"));
+ goto cleanup;
+ }
+ mirrorFormat = virXMLPropString(cur, "format");
+ }
+ if (mirrorFormat) {
+ def->mirror->format =
+ virStorageFileFormatTypeFromString(mirrorFormat);
+ if (def->mirror->format <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown mirror format value '%s'"),
+ mirrorFormat);
+ goto cleanup;
+ }
+ }
+ if (mirrorType) {
+ xmlNodePtr mirrorNode;
+
+ if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("mirror requires source element"));
+ goto cleanup;
+ }
+ if (virDomainDiskSourceParse(mirrorNode, ctxt,
+ def->mirror) < 0)
+ goto cleanup;
+ }
+ ready = virXMLPropString(cur, "ready");
+ if (ready) {
+ if ((def->mirrorState =
+ virDomainDiskMirrorStateTypeFromString(ready)) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unknown mirror ready state %s"),
+ ready);
+ VIR_FREE(ready);
+ goto cleanup;
+ }
+ VIR_FREE(ready);
+ }
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(mirrorType);
+ VIR_FREE(mirrorFormat);
+ return ret;
+}
+
+
#define VENDOR_LEN 8
#define PRODUCT_LEN 16
@@ -6799,8 +6902,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
char *vendor = NULL;
char *product = NULL;
char *discard = NULL;
- char *mirrorFormat = NULL;
- char *mirrorType = NULL;
char *domain_name = NULL;
int expected_secret_usage = -1;
int auth_secret_usage = -1;
@@ -6934,91 +7035,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
} else if (!def->mirror &&
xmlStrEqual(cur->name, BAD_CAST "mirror") &&
!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) {
- char *ready;
- char *blockJob;
-
- if (VIR_ALLOC(def->mirror) < 0)
+ if (virDomainDiskDefMirrorParse(def, cur, ctxt) < 0)
goto error;
-
- blockJob = virXMLPropString(cur, "job");
- if (blockJob) {
- def->mirrorJob = virDomainBlockJobTypeFromString(blockJob);
- if (def->mirrorJob <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror job type '%s'"),
- blockJob);
- VIR_FREE(blockJob);
- goto error;
- }
- VIR_FREE(blockJob);
- } else {
- def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
- }
-
- mirrorType = virXMLPropString(cur, "type");
- if (mirrorType) {
- def->mirror->type = virStorageTypeFromString(mirrorType);
- if (def->mirror->type <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror backing store "
- "type '%s'"), mirrorType);
- goto error;
- }
- mirrorFormat = virXPathString("string(./mirror/format/@type)",
- ctxt);
- } else {
- /* For back-compat reasons, we handle a file name
- * encoded as attributes, even though we prefer
- * modern output in the style of backingStore */
- def->mirror->type = VIR_STORAGE_TYPE_FILE;
- def->mirror->path = virXMLPropString(cur, "file");
- if (!def->mirror->path) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("mirror requires file name"));
- goto error;
- }
- if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("mirror without type only supported "
- "by copy job"));
- goto error;
- }
- mirrorFormat = virXMLPropString(cur, "format");
- }
- if (mirrorFormat) {
- def->mirror->format =
- virStorageFileFormatTypeFromString(mirrorFormat);
- if (def->mirror->format <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror format value
'%s'"),
- mirrorFormat);
- goto error;
- }
- }
- if (mirrorType) {
- xmlNodePtr mirrorNode;
-
- if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("mirror requires source element"));
- goto error;
- }
- if (virDomainDiskSourceParse(mirrorNode, ctxt,
- def->mirror) < 0)
- goto error;
- }
- ready = virXMLPropString(cur, "ready");
- if (ready) {
- if ((def->mirrorState =
- virDomainDiskMirrorStateTypeFromString(ready)) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("unknown mirror ready state %s"),
- ready);
- VIR_FREE(ready);
- goto error;
- }
- VIR_FREE(ready);
- }
} else if (!authdef &&
xmlStrEqual(cur->name, BAD_CAST "auth")) {
if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
@@ -7504,8 +7522,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(wwn);
VIR_FREE(vendor);
VIR_FREE(product);
- VIR_FREE(mirrorType);
- VIR_FREE(mirrorFormat);
VIR_FREE(domain_name);
ctxt->node = save_ctxt;
--
2.8.0
9:33 a.m.
New subject: [libvirt] [PATCH 08/10] conf: disk: Split out parsing of disk mirror data
On 04/12/2016 09:55 AM, Peter Krempa wrote:
Changes are indentation and 'cleanup' label instead of
'error'.
---
src/conf/domain_conf.c | 192 ++++++++++++++++++++++++++-----------------------
1 file changed, 104 insertions(+), 88 deletions(-)
ACK
- Cole
8:55 a.m.
New subject: [libvirt] [PATCH 09/10] conf: Refactor virDomainDiskDefMirrorParse
Now that the mirror parsing code is not crammed in the main disk parser
we can employ better coding style.
---
src/conf/domain_conf.c | 75 +++++++++++++++++++++-----------------------------
1 file changed, 31 insertions(+), 44 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 802a6fe..e64471d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6753,41 +6753,44 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def,
xmlNodePtr cur,
xmlXPathContextPtr ctxt)
{
+ xmlNodePtr mirrorNode;
char *mirrorFormat = NULL;
char *mirrorType = NULL;
- char *ready;
- char *blockJob;
+ char *ready = NULL;
+ char *blockJob = NULL;
int ret = -1;
if (VIR_ALLOC(def->mirror) < 0)
goto cleanup;
- blockJob = virXMLPropString(cur, "job");
- if (blockJob) {
- def->mirrorJob = virDomainBlockJobTypeFromString(blockJob);
- if (def->mirrorJob <= 0) {
+ if ((blockJob = virXMLPropString(cur, "job"))) {
+ if ((def->mirrorJob = virDomainBlockJobTypeFromString(blockJob)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror job type '%s'"),
- blockJob);
- VIR_FREE(blockJob);
+ _("unknown mirror job type '%s'"),
blockJob);
goto cleanup;
}
- VIR_FREE(blockJob);
} else {
def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
}
- mirrorType = virXMLPropString(cur, "type");
- if (mirrorType) {
- def->mirror->type = virStorageTypeFromString(mirrorType);
- if (def->mirror->type <= 0) {
+ if ((mirrorType = virXMLPropString(cur, "type"))) {
+ if ((def->mirror->type = virStorageTypeFromString(mirrorType)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror backing store "
- "type '%s'"), mirrorType);
+ _("unknown mirror backing store type
'%s'"),
+ mirrorType);
goto cleanup;
}
- mirrorFormat = virXPathString("string(./mirror/format/@type)",
- ctxt);
+
+ mirrorFormat = virXPathString("string(./mirror/format/@type)", ctxt);
+
+ if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("mirror requires source element"));
+ goto cleanup;
+ }
+
+ if (virDomainDiskSourceParse(mirrorNode, ctxt, def->mirror) < 0)
+ goto cleanup;
} else {
/* For back-compat reasons, we handle a file name
* encoded as attributes, even though we prefer
@@ -6807,44 +6810,28 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def,
}
mirrorFormat = virXMLPropString(cur, "format");
}
+
if (mirrorFormat) {
- def->mirror->format =
- virStorageFileFormatTypeFromString(mirrorFormat);
+ def->mirror->format = virStorageFileFormatTypeFromString(mirrorFormat);
if (def->mirror->format <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mirror format value '%s'"),
- mirrorFormat);
+ _("unknown mirror format value '%s'"),
mirrorFormat);
goto cleanup;
}
}
- if (mirrorType) {
- xmlNodePtr mirrorNode;
- if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("mirror requires source element"));
- goto cleanup;
- }
- if (virDomainDiskSourceParse(mirrorNode, ctxt,
- def->mirror) < 0)
- goto cleanup;
- }
- ready = virXMLPropString(cur, "ready");
- if (ready) {
- if ((def->mirrorState =
- virDomainDiskMirrorStateTypeFromString(ready)) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("unknown mirror ready state %s"),
- ready);
- VIR_FREE(ready);
- goto cleanup;
- }
- VIR_FREE(ready);
+ if ((ready = virXMLPropString(cur, "ready")) &&
+ (def->mirrorState = virDomainDiskMirrorStateTypeFromString(ready)) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unknown mirror ready state %s"), ready);
+ goto cleanup;
}
ret = 0;
cleanup:
+ VIR_FREE(ready);
+ VIR_FREE(blockJob);
VIR_FREE(mirrorType);
VIR_FREE(mirrorFormat);
return ret;
--
2.8.0
9:38 a.m.
New subject: [libvirt] [PATCH 09/10] conf: Refactor virDomainDiskDefMirrorParse
On 04/12/2016 09:55 AM, Peter Krempa wrote:
Now that the mirror parsing code is not crammed in the main disk
parser
we can employ better coding style.
---
src/conf/domain_conf.c | 75 +++++++++++++++++++++-----------------------------
1 file changed, 31 insertions(+), 44 deletions(-)
ACK
- Cole
8:55 a.m.
New subject: [libvirt] [PATCH 10/10] conf: extract disk geometry parsing code
---
src/conf/domain_conf.c | 75 +++++++++++++++++++++++++++++++-------------------
1 file changed, 46 insertions(+), 29 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e64471d..b5f2b91 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6838,6 +6838,51 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def,
}
+static int
+virDomainDiskDefGeometryParse(virDomainDiskDefPtr def,
+ xmlNodePtr cur,
+ xmlXPathContextPtr ctxt)
+{
+ char *trans;
+
+ if (virXPathUInt("string(./geometry/@cyls)",
+ ctxt, &def->geometry.cylinders) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid geometry settings (cyls)"));
+ return -1;
+ }
+
+ if (virXPathUInt("string(./geometry/@heads)",
+ ctxt, &def->geometry.heads) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid geometry settings (heads)"));
+ return -1;
+ }
+
+ if (virXPathUInt("string(./geometry/@secs)",
+ ctxt, &def->geometry.sectors) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid geometry settings (secs)"));
+ return -1;
+ }
+
+ trans = virXMLPropString(cur, "trans");
+ if (trans) {
+ def->geometry.trans = virDomainDiskGeometryTransTypeFromString(trans);
+ if (def->geometry.trans <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("invalid translation value '%s'"),
+ trans);
+ VIR_FREE(trans);
+ return -1;
+ }
+ VIR_FREE(trans);
+ }
+
+ return 0;
+}
+
+
#define VENDOR_LEN 8
#define PRODUCT_LEN 16
@@ -6866,7 +6911,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
char *driverType = NULL;
bool source = false;
char *target = NULL;
- char *trans = NULL;
char *bus = NULL;
char *cachetag = NULL;
char *error_policy = NULL;
@@ -6951,34 +6995,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
xmlStrEqual(cur->name, BAD_CAST "backenddomain")) {
domain_name = virXMLPropString(cur, "name");
} else if (xmlStrEqual(cur->name, BAD_CAST "geometry")) {
- if (virXPathUInt("string(./geometry/@cyls)",
- ctxt, &def->geometry.cylinders) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("invalid geometry settings (cyls)"));
+ if (virDomainDiskDefGeometryParse(def, cur, ctxt) < 0)
goto error;
- }
- if (virXPathUInt("string(./geometry/@heads)",
- ctxt, &def->geometry.heads) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("invalid geometry settings (heads)"));
- goto error;
- }
- if (virXPathUInt("string(./geometry/@secs)",
- ctxt, &def->geometry.sectors) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("invalid geometry settings (secs)"));
- goto error;
- }
- trans = virXMLPropString(cur, "trans");
- if (trans) {
- def->geometry.trans =
virDomainDiskGeometryTransTypeFromString(trans);
- if (def->geometry.trans <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("invalid translation value
'%s'"),
- trans);
- goto error;
- }
- }
} else if (xmlStrEqual(cur->name, BAD_CAST "blockio")) {
logical_block_size =
virXMLPropString(cur, "logical_block_size");
@@ -7486,7 +7504,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(target);
VIR_FREE(tray);
VIR_FREE(removable);
- VIR_FREE(trans);
VIR_FREE(device);
virStorageAuthDefFree(authdef);
VIR_FREE(driverType);
--
2.8.0
1:26 a.m.
New subject: [libvirt] [PATCH 10/10] conf: extract disk geometry parsing code
On Tue, Apr 12, 2016 at 10:38:40 -0400, Cole Robinson wrote:
On 04/12/2016 09:55 AM, Peter Krempa wrote:
> ---
> src/conf/domain_conf.c | 75 +++++++++++++++++++++++++++++++-------------------
> 1 file changed, 46 insertions(+), 29 deletions(-)
>
ACK
Thanks for a very swift review!! I've pushed these and I have a few more
to come later in this area.
Peter
3181
days inactive
3182
days old
24 comments
4 participants
participants (4)
-
Cole Robinson -
Jiri Denemark -
Ján Tomko -
Peter Krempa