[libvirt] [PATCH 00/14] conf: disk: Continue refactoring of virDomainDiskDefParseXML
This is yet another dump of my quest to despaghettize virDomainDiskDefParseXML. My eyes are starting to bleed a bit less, but there's still a lot of patches to come. Peter Krempa (14): conf: disk: Extract verification of disk config conf: disk: Split out parsing of disk <driver> element conf: disk: Avoid temporary variable when parsing driver name conf: disk: Sanitize parsing of disk format conf: disk: Mark VIR_DOMAIN_DISK_IO_DEFAULT as 0 and simplify parsing conf: disk: Remove custom single-use temporary variables conf: disk: Extract disk type and device right away conf: disk: Initialize closed device tray state to 0 conf: disk: Don't bother setting removable state to 0 by default conf: disk: extract sgio/rawio validation conf: disk: Extract verification of disk tray conf: disk: Move validation of disk bus vs disk type conf: disk: Extract checking of removable status conf: disk: extract validation of startup policy src/conf/domain_conf.c | 452 ++++++++++++++++++++++++------------------------- src/conf/domain_conf.h | 4 +- 2 files changed, 221 insertions(+), 235 deletions(-) -- 2.8.1
Rather than checking individual fields in dubious places extract them to a central point. --- src/conf/domain_conf.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28248c8..3e032e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6894,6 +6894,27 @@ virDomainDiskDefGeometryParse(virDomainDiskDefPtr def, } +static int +virDomainDiskDefValidate(const virDomainDiskDef *def) +{ + if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (def->event_idx != VIR_TRISTATE_SWITCH_ABSENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk event_idx mode supported only for virtio bus")); + return -1; + } + + if (def->ioeventfd != VIR_TRISTATE_SWITCH_ABSENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk ioeventfd mode supported only for virtio bus")); + return -1; + } + } + + return 0; +} + + #define VENDOR_LEN 8 #define PRODUCT_LEN 16 @@ -7365,13 +7386,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (ioeventfd) { int val; - if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk ioeventfd mode supported " - "only for virtio bus")); - goto error; - } - if ((val = virTristateSwitchTypeFromString(ioeventfd)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk ioeventfd mode '%s'"), @@ -7382,13 +7396,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } if (event_idx) { - if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk event_idx mode supported " - "only for virtio bus")); - goto error; - } - int idx; if ((idx = virTristateSwitchTypeFromString(event_idx)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -7506,6 +7513,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } + if (virDomainDiskDefValidate(def) < 0) + goto error; + cleanup: VIR_FREE(bus); VIR_FREE(type); -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:11PM +0200, Peter Krempa wrote:
Rather than checking individual fields in dubious places extract them to a central point. --- src/conf/domain_conf.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)
ACK Jan
--- src/conf/domain_conf.c | 291 ++++++++++++++++++++++++++----------------------- 1 file changed, 154 insertions(+), 137 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e032e8..43a90a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6915,6 +6915,157 @@ virDomainDiskDefValidate(const virDomainDiskDef *def) } +static int +virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, + xmlNodePtr cur) +{ + char *driverName = NULL; + char *driverType = NULL; + char *cachetag = NULL; + char *error_policy = NULL; + char *rerror_policy = NULL; + char *iotag = NULL; + char *ioeventfd = NULL; + char *event_idx = NULL; + char *copy_on_read = NULL; + char *discard = NULL; + char *driverIOThread = NULL; + int ret = -1; + + 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"); + + def->src->driverName = driverName; + driverName = NULL; + + if (cachetag && + (def->cachemode = virDomainDiskCacheTypeFromString(cachetag)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk cache mode '%s'"), cachetag); + goto cleanup; + } + + if (error_policy && + (def->error_policy = virDomainDiskErrorPolicyTypeFromString(error_policy)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk error policy '%s'"), error_policy); + goto cleanup; + } + + if (rerror_policy && + (((def->rerror_policy + = virDomainDiskErrorPolicyTypeFromString(rerror_policy)) <= 0) || + (def->rerror_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk read error policy '%s'"), + rerror_policy); + goto cleanup; + } + + if (iotag) { + if ((def->iomode = virDomainDiskIoTypeFromString(iotag)) < 0 || + def->iomode == VIR_DOMAIN_DISK_IO_DEFAULT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk io mode '%s'"), iotag); + goto cleanup; + } + } + + if (ioeventfd) { + int val; + if ((val = virTristateSwitchTypeFromString(ioeventfd)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk ioeventfd mode '%s'"), + ioeventfd); + goto cleanup; + } + def->ioeventfd = val; + } + + if (event_idx) { + int idx; + if ((idx = virTristateSwitchTypeFromString(event_idx)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk event_idx mode '%s'"), + event_idx); + goto cleanup; + } + def->event_idx = idx; + } + + if (copy_on_read) { + int cor; + if ((cor = virTristateSwitchTypeFromString(copy_on_read)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk copy_on_read mode '%s'"), + copy_on_read); + goto cleanup; + } + def->copy_on_read = cor; + } + + if (discard) { + if ((def->discard = virDomainDiskDiscardTypeFromString(discard)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk discard mode '%s'"), discard); + goto cleanup; + } + } + + if (driverIOThread) { + if (virStrToLong_uip(driverIOThread, NULL, 10, &def->iothread) < 0 || + def->iothread == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid iothread attribute in disk driver " + "element: %s"), driverIOThread); + goto cleanup; + } + } + + if (driverType) { + def->src->format = virStorageFileFormatTypeFromString(driverType); + if (def->src->format <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown driver format value '%s'"), + driverType); + goto cleanup; + } + } + + ret = 0; + + cleanup: + VIR_FREE(driverType); + VIR_FREE(driverName); + VIR_FREE(cachetag); + VIR_FREE(error_policy); + VIR_FREE(rerror_policy); + VIR_FREE(iotag); + VIR_FREE(ioeventfd); + VIR_FREE(event_idx); + VIR_FREE(copy_on_read); + VIR_FREE(discard); + VIR_FREE(driverIOThread); + + return ret; +} + + #define VENDOR_LEN 8 #define PRODUCT_LEN 16 @@ -6939,19 +7090,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *snapshot = NULL; char *rawio = NULL; char *sgio = NULL; - char *driverName = NULL; - char *driverType = NULL; bool source = false; char *target = NULL; char *bus = NULL; - char *cachetag = NULL; - char *error_policy = NULL; - char *rerror_policy = NULL; - char *iotag = NULL; - char *ioeventfd = NULL; - char *event_idx = NULL; - char *copy_on_read = NULL; - char *driverIOThread = NULL; char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; char *serial = NULL; @@ -6964,7 +7105,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *wwn = NULL; char *vendor = NULL; char *product = NULL; - char *discard = NULL; char *domain_name = NULL; int expected_secret_usage = -1; int auth_secret_usage = -1; @@ -7050,25 +7190,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, physical_block_size); goto error; } - } else if (!driverName && + } else if (!def->src->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"); + if (virDomainDiskDefDriverParseXML(def, cur) < 0) + goto error; } else if (!def->mirror && xmlStrEqual(cur->name, BAD_CAST "mirror") && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) { @@ -7350,91 +7475,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (cachetag && - (def->cachemode = virDomainDiskCacheTypeFromString(cachetag)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk cache mode '%s'"), cachetag); - goto error; - } - - if (error_policy && - (def->error_policy = virDomainDiskErrorPolicyTypeFromString(error_policy)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk error policy '%s'"), error_policy); - goto error; - } - - if (rerror_policy && - (((def->rerror_policy - = virDomainDiskErrorPolicyTypeFromString(rerror_policy)) <= 0) || - (def->rerror_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk read error policy '%s'"), - rerror_policy); - goto error; - } - - if (iotag) { - if ((def->iomode = virDomainDiskIoTypeFromString(iotag)) < 0 || - def->iomode == VIR_DOMAIN_DISK_IO_DEFAULT) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk io mode '%s'"), iotag); - goto error; - } - } - - if (ioeventfd) { - int val; - - if ((val = virTristateSwitchTypeFromString(ioeventfd)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk ioeventfd mode '%s'"), - ioeventfd); - goto error; - } - def->ioeventfd = val; - } - - if (event_idx) { - int idx; - if ((idx = virTristateSwitchTypeFromString(event_idx)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk event_idx mode '%s'"), - event_idx); - goto error; - } - def->event_idx = idx; - } - - if (copy_on_read) { - int cor; - if ((cor = virTristateSwitchTypeFromString(copy_on_read)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk copy_on_read mode '%s'"), - copy_on_read); - goto error; - } - def->copy_on_read = cor; - } - - if (discard) { - if ((def->discard = virDomainDiskDiscardTypeFromString(discard)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk discard mode '%s'"), discard); - goto error; - } - } - - if (driverIOThread) { - if (virStrToLong_uip(driverIOThread, NULL, 10, &def->iothread) < 0 || - def->iothread == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid iothread attribute in disk driver " - "element: %s"), driverIOThread); - goto error; - } - } - if (devaddr) { if (virDomainParseLegacyDeviceAddress(devaddr, &def->info.addr.pci) < 0) { @@ -7483,8 +7523,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, target = NULL; def->src->auth = authdef; authdef = NULL; - def->src->driverName = driverName; - driverName = NULL; def->src->encryption = encryption; encryption = NULL; def->domain_name = domain_name; @@ -7498,16 +7536,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->product = product; product = NULL; - if (driverType) { - def->src->format = virStorageFileFormatTypeFromString(driverType); - if (def->src->format <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown driver format value '%s'"), - driverType); - goto error; - } - } - if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) goto error; @@ -7527,17 +7555,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(removable); VIR_FREE(device); virStorageAuthDefFree(authdef); - VIR_FREE(driverType); - VIR_FREE(driverName); - VIR_FREE(cachetag); - VIR_FREE(error_policy); - VIR_FREE(rerror_policy); - VIR_FREE(iotag); - VIR_FREE(ioeventfd); - VIR_FREE(event_idx); - VIR_FREE(copy_on_read); - VIR_FREE(discard); - VIR_FREE(driverIOThread); VIR_FREE(devaddr); VIR_FREE(serial); virStorageEncryptionFree(encryption); -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:12PM +0200, Peter Krempa wrote:
--- src/conf/domain_conf.c | 291 ++++++++++++++++++++++++++----------------------- 1 file changed, 154 insertions(+), 137 deletions(-)
ACK Jan
--- src/conf/domain_conf.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 43a90a3..21a904d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6919,7 +6919,6 @@ static int virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, xmlNodePtr cur) { - char *driverName = NULL; char *driverType = NULL; char *cachetag = NULL; char *error_policy = NULL; @@ -6932,7 +6931,7 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, char *driverIOThread = NULL; int ret = -1; - driverName = virXMLPropString(cur, "name"); + def->src->driverName = virXMLPropString(cur, "name"); driverType = virXMLPropString(cur, "type"); if (STREQ_NULLABLE(driverType, "aio")) { /* In-place conversion to "raw", for Xen back-compat */ @@ -6950,9 +6949,6 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, discard = virXMLPropString(cur, "discard"); driverIOThread = virXMLPropString(cur, "iothread"); - def->src->driverName = driverName; - driverName = NULL; - if (cachetag && (def->cachemode = virDomainDiskCacheTypeFromString(cachetag)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -7051,7 +7047,6 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, cleanup: VIR_FREE(driverType); - VIR_FREE(driverName); VIR_FREE(cachetag); VIR_FREE(error_policy); VIR_FREE(rerror_policy); -- 2.8.1
--- src/conf/domain_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 21a904d..5ac4beb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6919,7 +6919,7 @@ static int virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, xmlNodePtr cur) { - char *driverType = NULL; + char *tmp = NULL; char *cachetag = NULL; char *error_policy = NULL; char *rerror_policy = NULL; @@ -6932,13 +6932,6 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, int ret = -1; def->src->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"); @@ -7033,20 +7026,25 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, } } - if (driverType) { - def->src->format = virStorageFileFormatTypeFromString(driverType); - if (def->src->format <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown driver format value '%s'"), - driverType); - goto cleanup; + if ((tmp = virXMLPropString(cur, "type"))) { + if (STREQ(tmp, "aio")) { + /* Xen back-compat */ + def->src->format = VIR_STORAGE_FILE_RAW; + } else { + if ((def->src->format = virStorageFileFormatTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown driver format value '%s'"), tmp); + goto cleanup; + } } + + VIR_FREE(tmp); } ret = 0; cleanup: - VIR_FREE(driverType); + VIR_FREE(tmp); VIR_FREE(cachetag); VIR_FREE(error_policy); VIR_FREE(rerror_policy); -- 2.8.1
--- src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ac4beb..9c4edfa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6967,8 +6967,7 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, } if (iotag) { - if ((def->iomode = virDomainDiskIoTypeFromString(iotag)) < 0 || - def->iomode == VIR_DOMAIN_DISK_IO_DEFAULT) { + if ((def->iomode = virDomainDiskIoTypeFromString(iotag)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk io mode '%s'"), iotag); goto cleanup; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1986f53..555d45d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -622,7 +622,7 @@ typedef enum { } virDomainDiskGeometryTrans; typedef enum { - VIR_DOMAIN_DISK_IO_DEFAULT, + VIR_DOMAIN_DISK_IO_DEFAULT = 0, VIR_DOMAIN_DISK_IO_NATIVE, VIR_DOMAIN_DISK_IO_THREADS, -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:15PM +0200, Peter Krempa wrote:
--- src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
ACK Jan
Use a single temporary variable instead shortening the code. --- src/conf/domain_conf.c | 137 ++++++++++++++++++------------------------------- 1 file changed, 51 insertions(+), 86 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9c4edfa..4267372 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6920,110 +6920,84 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, xmlNodePtr cur) { char *tmp = NULL; - char *cachetag = NULL; - char *error_policy = NULL; - char *rerror_policy = NULL; - char *iotag = NULL; - char *ioeventfd = NULL; - char *event_idx = NULL; - char *copy_on_read = NULL; - char *discard = NULL; - char *driverIOThread = NULL; int ret = -1; def->src->driverName = virXMLPropString(cur, "name"); - 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"); - - if (cachetag && - (def->cachemode = virDomainDiskCacheTypeFromString(cachetag)) < 0) { + + if ((tmp = virXMLPropString(cur, "cache")) && + (def->cachemode = virDomainDiskCacheTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk cache mode '%s'"), cachetag); + _("unknown disk cache mode '%s'"), tmp); goto cleanup; } + VIR_FREE(tmp); - if (error_policy && - (def->error_policy = virDomainDiskErrorPolicyTypeFromString(error_policy)) <= 0) { + if ((tmp = virXMLPropString(cur, "error_policy")) && + (def->error_policy = virDomainDiskErrorPolicyTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk error policy '%s'"), error_policy); + _("unknown disk error policy '%s'"), tmp); goto cleanup; } + VIR_FREE(tmp); - if (rerror_policy && - (((def->rerror_policy - = virDomainDiskErrorPolicyTypeFromString(rerror_policy)) <= 0) || + if ((tmp = virXMLPropString(cur, "rerror_policy")) && + (((def->rerror_policy = virDomainDiskErrorPolicyTypeFromString(tmp)) <= 0) || (def->rerror_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk read error policy '%s'"), - rerror_policy); + _("unknown disk read error policy '%s'"), tmp); goto cleanup; } + VIR_FREE(tmp); - if (iotag) { - if ((def->iomode = virDomainDiskIoTypeFromString(iotag)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk io mode '%s'"), iotag); - goto cleanup; - } + if ((tmp = virXMLPropString(cur, "io")) && + (def->iomode = virDomainDiskIoTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk io mode '%s'"), tmp); + goto cleanup; } + VIR_FREE(tmp); - if (ioeventfd) { - int val; - if ((val = virTristateSwitchTypeFromString(ioeventfd)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk ioeventfd mode '%s'"), - ioeventfd); - goto cleanup; - } - def->ioeventfd = val; + if ((tmp = virXMLPropString(cur, "ioeventfd")) && + (def->ioeventfd = virTristateSwitchTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk ioeventfd mode '%s'"), tmp); + goto cleanup; } + VIR_FREE(tmp); - if (event_idx) { - int idx; - if ((idx = virTristateSwitchTypeFromString(event_idx)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk event_idx mode '%s'"), - event_idx); - goto cleanup; - } - def->event_idx = idx; + if ((tmp = virXMLPropString(cur, "event_idx")) && + (def->event_idx = virTristateSwitchTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk event_idx mode '%s'"), tmp); + goto cleanup; } + VIR_FREE(tmp); - if (copy_on_read) { - int cor; - if ((cor = virTristateSwitchTypeFromString(copy_on_read)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk copy_on_read mode '%s'"), - copy_on_read); - goto cleanup; - } - def->copy_on_read = cor; + if ((tmp = virXMLPropString(cur, "copy_on_read")) && + (def->copy_on_read = virTristateSwitchTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk copy_on_read mode '%s'"), tmp); + goto cleanup; } + VIR_FREE(tmp); - if (discard) { - if ((def->discard = virDomainDiskDiscardTypeFromString(discard)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk discard mode '%s'"), discard); - goto cleanup; - } + if ((tmp = virXMLPropString(cur, "discard")) && + (def->discard = virDomainDiskDiscardTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk discard mode '%s'"), tmp); + goto cleanup; } + VIR_FREE(tmp); - if (driverIOThread) { - if (virStrToLong_uip(driverIOThread, NULL, 10, &def->iothread) < 0 || - def->iothread == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid iothread attribute in disk driver " - "element: %s"), driverIOThread); - goto cleanup; - } + if ((tmp = virXMLPropString(cur, "iothread")) && + (virStrToLong_uip(tmp, NULL, 10, &def->iothread) < 0 || + def->iothread == 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid iothread attribute in disk driver element: %s"), + tmp); + goto cleanup; } + VIR_FREE(tmp); if ((tmp = virXMLPropString(cur, "type"))) { if (STREQ(tmp, "aio")) { @@ -7044,15 +7018,6 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def, cleanup: VIR_FREE(tmp); - VIR_FREE(cachetag); - VIR_FREE(error_policy); - VIR_FREE(rerror_policy); - VIR_FREE(iotag); - VIR_FREE(ioeventfd); - VIR_FREE(event_idx); - VIR_FREE(copy_on_read); - VIR_FREE(discard); - VIR_FREE(driverIOThread); return ret; } -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:16PM +0200, Peter Krempa wrote:
Use a single temporary variable instead shortening the code. --- src/conf/domain_conf.c | 137 ++++++++++++++++++------------------------------- 1 file changed, 51 insertions(+), 86 deletions(-)
ACK Jan
Additionally switch to using a common temp variable for the xml elements. --- src/conf/domain_conf.c | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4267372..92a7731 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7042,8 +7042,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr sourceNode = NULL; xmlNodePtr cur; xmlNodePtr save_ctxt = ctxt->node; - char *type = NULL; - char *device = NULL; + char *tmp = NULL; char *snapshot = NULL; char *rawio = NULL; char *sgio = NULL; @@ -7071,16 +7070,25 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = node; - type = virXMLPropString(node, "type"); - if (type) { - if ((def->src->type = virStorageTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk type '%s'"), type); - goto error; - } - } else { - def->src->type = VIR_STORAGE_TYPE_FILE; + /* defaults */ + def->src->type = VIR_STORAGE_TYPE_FILE; + def->device = VIR_DOMAIN_DISK_DEVICE_DISK; + + if ((tmp = virXMLPropString(node, "type")) && + (def->src->type = virStorageTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk type '%s'"), tmp); + goto error; } + VIR_FREE(tmp); + + if ((tmp = virXMLPropString(node, "device")) && + (def->device = virDomainDiskDeviceTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk device '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); snapshot = virXMLPropString(node, "snapshot"); @@ -7245,16 +7253,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - device = virXMLPropString(node, "device"); - if (device) { - if ((def->device = virDomainDiskDeviceTypeFromString(device)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk device '%s'"), device); - goto error; - } - } else { - def->device = VIR_DOMAIN_DISK_DEVICE_DISK; - } /* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present. LUN is for raw access CD-ROMs @@ -7283,7 +7281,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (!target && !(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { if (def->src->srcpool) { - char *tmp; if (virAsprintf(&tmp, "pool = '%s', volume = '%s'", def->src->srcpool->pool, def->src->srcpool->volume) < 0) goto error; @@ -7502,15 +7499,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; cleanup: + VIR_FREE(tmp); VIR_FREE(bus); - VIR_FREE(type); VIR_FREE(snapshot); VIR_FREE(rawio); VIR_FREE(sgio); VIR_FREE(target); VIR_FREE(tray); VIR_FREE(removable); - VIR_FREE(device); virStorageAuthDefFree(authdef); VIR_FREE(devaddr); VIR_FREE(serial); -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:17PM +0200, Peter Krempa wrote:
Additionally switch to using a common temp variable for the xml elements. --- src/conf/domain_conf.c | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-)
ACK Jan
Additionally avoid initializing it after being calloced. --- src/conf/domain_conf.c | 4 ---- src/conf/domain_conf.h | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 92a7731..a1febda 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7393,10 +7393,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("tray is only valid for cdrom and floppy")); goto error; } - } else { - if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY || - def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) - def->tray_status = VIR_DOMAIN_DISK_TRAY_CLOSED; } if (removable) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 555d45d..fd540ed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -606,7 +606,7 @@ typedef enum { typedef enum { - VIR_DOMAIN_DISK_TRAY_CLOSED, + VIR_DOMAIN_DISK_TRAY_CLOSED = 0, VIR_DOMAIN_DISK_TRAY_OPEN, VIR_DOMAIN_DISK_TRAY_LAST -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:18PM +0200, Peter Krempa wrote:
Additionally avoid initializing it after being calloced. --- src/conf/domain_conf.c | 4 ---- src/conf/domain_conf.h | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-)
ACK Jan
--- src/conf/domain_conf.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a1febda..b1432ef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7407,9 +7407,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("removable is only valid for usb disks")); goto error; } - } else { - if (def->bus == VIR_DOMAIN_DISK_BUS_USB) - def->removable = VIR_TRISTATE_SWITCH_ABSENT; } if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && -- 2.8.1
--- src/conf/domain_conf.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1432ef..8ac308c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6911,6 +6911,20 @@ virDomainDiskDefValidate(const virDomainDiskDef *def) } } + if (def->device != VIR_DOMAIN_DISK_DEVICE_LUN) { + if (def->rawio != VIR_TRISTATE_BOOL_ABSENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("rawio can be used only with device='lun'")); + return -1; + } + + if (def->sgio != VIR_DOMAIN_DEVICE_SGIO_DEFAULT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("sgio can be used only with device='lun'")); + return -1; + } + } + return 0; } @@ -7330,14 +7344,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } - if ((rawio || sgio) && - (def->device != VIR_DOMAIN_DISK_DEVICE_LUN)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("rawio or sgio can be used only with " - "device='lun'")); - goto error; - } - if (rawio) { if ((def->rawio = virTristateBoolTypeFromString(rawio)) <= 0) { virReportError(VIR_ERR_XML_ERROR, -- 2.8.1
--- src/conf/domain_conf.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8ac308c..5fd8cc4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6925,6 +6925,14 @@ virDomainDiskDefValidate(const virDomainDiskDef *def) } } + if (def->tray_status != VIR_DOMAIN_DISK_TRAY_CLOSED && + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("tray is only valid for cdrom and floppy")); + return -1; + } + return 0; } @@ -7392,13 +7400,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("unknown disk tray status '%s'"), tray); goto error; } - - if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("tray is only valid for cdrom and floppy")); - goto error; - } } if (removable) { -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:21PM +0200, Peter Krempa wrote:
--- src/conf/domain_conf.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
This will let you specify tray='closed' for non-removable media too. The previous patch has a similar issue, but sgio="default" for a disk that does not support it looks a bit less odd than tray="closed" for a hard drive. Jan
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8ac308c..5fd8cc4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6925,6 +6925,14 @@ virDomainDiskDefValidate(const virDomainDiskDef *def) } }
+ if (def->tray_status != VIR_DOMAIN_DISK_TRAY_CLOSED && + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("tray is only valid for cdrom and floppy")); + return -1; + } + return 0; }
@@ -7392,13 +7400,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("unknown disk tray status '%s'"), tray); goto error; } - - if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("tray is only valid for cdrom and floppy")); - goto error; - } }
if (removable) { -- 2.8.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
--- src/conf/domain_conf.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5fd8cc4..baf6f84 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6933,6 +6933,22 @@ virDomainDiskDefValidate(const virDomainDiskDef *def) return -1; } + if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && + def->bus != VIR_DOMAIN_DISK_BUS_FDC) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid bus type '%s' for floppy disk"), + virDomainDiskBusTypeToString(def->bus)); + return -1; + } + + if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + def->bus == VIR_DOMAIN_DISK_BUS_FDC) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid bus type '%s' for disk"), + virDomainDiskBusTypeToString(def->bus)); + return -1; + } + return 0; } @@ -7416,19 +7432,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && - def->bus != VIR_DOMAIN_DISK_BUS_FDC) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid bus type '%s' for floppy disk"), bus); - goto error; - } - if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - def->bus == VIR_DOMAIN_DISK_BUS_FDC) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid bus type '%s' for disk"), bus); - goto error; - } - if (devaddr) { if (virDomainParseLegacyDeviceAddress(devaddr, &def->info.addr.pci) < 0) { -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:22PM +0200, Peter Krempa wrote:
--- src/conf/domain_conf.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
ACK Jan
--- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index baf6f84..2456639 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6949,6 +6949,13 @@ virDomainDiskDefValidate(const virDomainDiskDef *def) return -1; } + if (def->removable != VIR_TRISTATE_SWITCH_ABSENT && + def->bus != VIR_DOMAIN_DISK_BUS_USB) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("removable is only valid for usb disks")); + return -1; + } + return 0; } @@ -7424,12 +7431,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("unknown disk removable status '%s'"), removable); goto error; } - - if (def->bus != VIR_DOMAIN_DISK_BUS_USB) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("removable is only valid for usb disks")); - goto error; - } } if (devaddr) { -- 2.8.1
--- src/conf/domain_conf.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2456639..4e6b82f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6956,6 +6956,25 @@ virDomainDiskDefValidate(const virDomainDiskDef *def) return -1; } + if (def->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_DEFAULT) { + if (def->src->type == VIR_STORAGE_TYPE_NETWORK) { + virReportError(VIR_ERR_XML_ERROR, + _("Setting disk %s is not allowed for " + "disk of network type"), + virDomainStartupPolicyTypeToString(def->startupPolicy)); + return -1; + } + + if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + def->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Setting disk 'requisite' is allowed only for " + "cdrom or floppy")); + return -1; + } + } + return 0; } @@ -7457,23 +7476,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, startupPolicy); goto error; } - - if (def->src->type == VIR_STORAGE_TYPE_NETWORK) { - virReportError(VIR_ERR_XML_ERROR, - _("Setting disk %s is not allowed for " - "disk of network type"), - startupPolicy); - goto error; - } - - if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - val == VIR_DOMAIN_STARTUP_POLICY_REQUISITE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Setting disk 'requisite' is allowed only for " - "cdrom or floppy")); - goto error; - } def->startupPolicy = val; } -- 2.8.1
On Wed, Apr 20, 2016 at 05:58:24PM +0200, Peter Krempa wrote:
--- src/conf/domain_conf.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
ACK Jan
participants (2)
-
Ján Tomko -
Peter Krempa