[libvirt] [PATCH 00/10] conf: disk xml parser refactors
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
--- 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
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
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
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
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
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
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
--- 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
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
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
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
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
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
s/simplfiy/simplify/ in the commit summary. Jan
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
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
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
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
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
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
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
--- 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
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
participants (4)
-
Cole Robinson -
Jiri Denemark -
Ján Tomko -
Peter Krempa