[libvirt] [PATCH 00/34] conf: refactor virStorageSource parsing and formatting (blockdev-add saga)
Simplify parsing and formatting of virStorageSource into/from XML. This series contains the refactors which unify the various parsers into one with a neat cleanup. The plan is to reuse the parser for parsing job backing chains when the disk frontend will be unplugged by the guest so that we don't lose track of the backing chain members. This series adds support for backing chain of the <mirror> subelement which has a similar reason. If we open a user-formatted file (--reuse-external) and do a shallow copy into it we will need to keep around the backing chain for that as well. It feels fitting to format the backing chain under <mirror> as it might become the disk source later. Peter Krempa (34): conf: Invert 'skipSeclabels' argument of virDomainDiskSourceFormatInternal conf: Move formatting of 'index' and 'startupPolicy' for virStorageSource conf: Introduce virDomainStorageSourceFormatFull qemu: domain: Replace qemuDomainObjPrivateXMLFormatNBDMigrationSource conf: Unexport virDomainStorageSourceFormat conf: domain: Merge virDomainDiskSourceFormatInternal into the wrapper conf: Simplify control flow in virDomainDiskSourceFormat conf: Avoid temporary variable in virDomainDiskBackingStoreFormat conf: Use virXMLFormatElement in virDomainDiskBackingStoreFormat conf: Move virDomainDiskBackingStoreFormat up to avoid forward declarations conf: Move backingStore formating into virDomainDiskSourceFormat conf: Add possibility to format full chain with virDomainStorageSourceFormatFull conf: Simplify error paths in storage source component parsers cleanup error path in virDomainStorageSourceParse util: xml: Introduce VIR_AUTOPTR functions for xmlDoc and xmlXPathContext tests: Use full force of our VIR_AUTO* machinery in testBackingXMLjsonXML tests: Refactor control flow in testBackingXMLjsonXML conf: Refactor control flow in virDomainDiskBackingStoreParse conf: Fold private data parsing into virDomainStorageSourceParse conf: Introduce modular parser for virStorageSource qemu: Use virDomainStorageSourceParseFull when parsing NBD migration data conf: Unexport virDomainStorageSourceParse tests: qemublock: Use new source formatter and parser in testBackingXMLjsonXML conf: snapshot: Use virDomainStorageSourceParseFull for snapshots conf: Replace virDomainDiskSourceParse by virDomainStorageSourceParse conf: Use virDomainStorageSourceParseFull in virDomainDiskBackingStoreParse conf: Use virDomainStorageSourceParseFull to parse disk source in virDomainDiskDefParseXML conf: Use virDomainStorageSourceParseFull in virDomainDiskDefMirrorParse conf: Parse <backingStore> in virDomainStorageSourceParseFull conf: Parse and format 'backingStore' for disk <mirror> conf: Add 'index' attribute for <disk><mirror><source> conf: Format seclabels for <backingStore> conf: Remove @seclabels from virDomainDiskSourceFormat conf: Remove @seclabels from virDomainStorageSourceFormat docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 494 ++++++++++-------- src/conf/domain_conf.h | 32 +- src/conf/snapshot_conf.c | 40 +- src/libvirt_private.syms | 4 +- src/qemu/qemu_domain.c | 104 +--- src/util/virxml.h | 3 + tests/qemublocktest.c | 179 +++---- .../blockjob-mirror-in.xml | 13 + .../qemuxml2argvdata/disk-backing-chains.xml | 6 +- tests/qemuxml2argvdata/disk-mirror.xml | 8 +- .../disk-backing-chains-active.xml | 6 +- .../disk-backing-chains-inactive.xml | 6 +- .../qemuxml2xmloutdata/disk-mirror-active.xml | 8 +- tests/virstoragetest.c | 2 +- 15 files changed, 443 insertions(+), 463 deletions(-) -- 2.20.1
Rename it to 'seclabels' and invert the value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 504c24b545..97d56c0067 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23760,7 +23760,7 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, unsigned int flags, - bool skipSeclabels) + bool seclabels) { switch ((virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: @@ -23800,7 +23800,7 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, return -1; } - if (!skipSeclabels && src->type != VIR_STORAGE_TYPE_NETWORK) + if (seclabels && src->type != VIR_STORAGE_TYPE_NETWORK) virDomainSourceDefFormatSeclabel(childBuf, src->nseclabels, src->seclabels, flags); @@ -23832,7 +23832,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virStorageSourcePtr src, int policy, unsigned int flags, - bool skipSeclabels, + bool seclabels, bool attrIndex, virDomainXMLOptionPtr xmlopt) { @@ -23843,7 +23843,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virBufferSetChildIndent(&childBuf, buf); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - skipSeclabels) < 0) + seclabels) < 0) goto cleanup; if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) @@ -23873,7 +23873,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, unsigned int flags, virDomainXMLOptionPtr xmlopt) { - return virDomainDiskSourceFormatInternal(buf, src, policy, flags, false, + return virDomainDiskSourceFormatInternal(buf, src, policy, flags, true, false, xmlopt); } @@ -23916,7 +23916,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAsprintf(buf, "<format type='%s'/>\n", format); /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, true, + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, false, false, xmlopt) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, xmlopt, flags) < 0) @@ -24175,7 +24175,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virStorageAuthDefFormat(buf, def->src->auth); if (virDomainDiskSourceFormatInternal(buf, def->src, def->startupPolicy, - flags, false, true, xmlopt) < 0) + flags, true, true, xmlopt) < 0) return -1; /* Don't format backingStore to inactive XMLs until the code for diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 538fb50b9e..cad330715b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3461,7 +3461,7 @@ int virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, unsigned int flags, - bool skipSeclabels) + bool seclabels) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virDomainStorageSourceParse(xmlNodePtr node, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 86e80391e1..341ea7d37c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2358,7 +2358,7 @@ qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, virStorageFileFormatTypeToString(src->format)); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, - VIR_DOMAIN_DEF_FORMAT_STATUS, false) < 0) + VIR_DOMAIN_DEF_FORMAT_STATUS, true) < 0) goto cleanup; if (qemuStorageSourcePrivateDataFormat(src, &privateDataBuf) < 0) -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:50PM +0100, Peter Krempa wrote:
Rename it to 'seclabels' and invert the value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move the formatters from virDomainDiskSourceFormatInternal to virDomainStorageSourceFormat. While 'startupPolicy' is attribute of the disk, we can format it when formating generic virStorageSource into an element to allow simplifying the code. Since the arguments for virDomainStorageSourceFormat got complex this patch also documents them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++------------ src/conf/domain_conf.h | 5 ++++- src/qemu/qemu_domain.c | 3 ++- 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 97d56c0067..4083839fc8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23755,12 +23755,32 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, } +/** + * virDomainStorageSourceFormat: + * @attrBuf: buffer for containing attribute portion of @src + * @childBuf: buffer for subelements of the formatted element + * @src: storage source to format + * @flags: XML formatter flags + * @seclabels: security labels are formatted if true + * @attrIndex: the 'index' attribute is formatted if true + * @policy: startup policy, taken from disk (use 0 to omit) + * @xmlopt: XML options data (for private data formatters) + * + * Formats @src into the attributes (@attrBuf) and subelements (@childBuf) ready + * for creating a full XML element representing @src. + * + * Note that this does _not_ format the 'type' and 'format' of @src due to + * differences in callers. + */ int virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, unsigned int flags, - bool seclabels) + bool seclabels, + bool attrIndex, + int policy, + virDomainXMLOptionPtr xmlopt) { switch ((virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: @@ -23823,6 +23843,16 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, virStoragePRDefFormat(childBuf, src->pr, flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE); + if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) + virBufferEscapeString(attrBuf, " startupPolicy='%s'", + virDomainStartupPolicyTypeToString(policy)); + + if (attrIndex && src->id != 0) + virBufferAsprintf(attrBuf, " index='%u'", src->id); + + if (virDomainDiskSourceFormatPrivateData(childBuf, src, flags, xmlopt) < 0) + return -1; + return 0; } @@ -23843,17 +23873,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virBufferSetChildIndent(&childBuf, buf); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - seclabels) < 0) - goto cleanup; - - if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", - virDomainStartupPolicyTypeToString(policy)); - - if (attrIndex && src->id != 0) - virBufferAsprintf(&attrBuf, " index='%u'", src->id); - - if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) + seclabels, attrIndex, policy, xmlopt) < 0) goto cleanup; if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cad330715b..a6f8e13088 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3461,7 +3461,10 @@ int virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, unsigned int flags, - bool seclabels) + bool seclabels, + bool attrIndex, + int policy, + virDomainXMLOptionPtr xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virDomainStorageSourceParse(xmlNodePtr node, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 341ea7d37c..d25d9ab666 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2358,7 +2358,8 @@ qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, virStorageFileFormatTypeToString(src->format)); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, - VIR_DOMAIN_DEF_FORMAT_STATUS, true) < 0) + VIR_DOMAIN_DEF_FORMAT_STATUS, true, + false, 0, NULL) < 0) goto cleanup; if (qemuStorageSourcePrivateDataFormat(src, &privateDataBuf) < 0) -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:51PM +0100, Peter Krempa wrote:
Move the formatters from virDomainDiskSourceFormatInternal to virDomainStorageSourceFormat.
While 'startupPolicy' is attribute of the disk, we can format it when formating generic virStorageSource into an element to allow simplifying the code.
All but one callers pass 0, is it really worth putting a non-source attribute here?
Since the arguments for virDomainStorageSourceFormat got complex this patch also documents them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++------------ src/conf/domain_conf.h | 5 ++++- src/qemu/qemu_domain.c | 3 ++- 3 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 97d56c0067..4083839fc8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23755,12 +23755,32 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, }
+/** + * virDomainStorageSourceFormat: + * @attrBuf: buffer for containing attribute portion of @src + * @childBuf: buffer for subelements of the formatted element + * @src: storage source to format + * @flags: XML formatter flags + * @seclabels: security labels are formatted if true + * @attrIndex: the 'index' attribute is formatted if true + * @policy: startup policy, taken from disk (use 0 to omit) + * @xmlopt: XML options data (for private data formatters) + * + * Formats @src into the attributes (@attrBuf) and subelements (@childBuf) ready + * for creating a full XML element representing @src. + * + * Note that this does _not_ format the 'type' and 'format' of @src due to + * differences in callers.
different needs of callers.
+ */ int
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The new function formats a virStorageSource into an XML element which already contains type and format for simpler handling. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 48 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4083839fc8..9f1f46c905 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23857,6 +23857,45 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, } +/** + * virDomainStorageSourceFormatFull: + * @buf: output buffer + * @src: storage source to format + * @elemname: name of the top level element to use + * @status: output status-XML style private data + * @xmlopt: formatter callback data structure + * + * Formats @src into a XML element called @elemname. The element has both 'type' + * and 'format' attributes and thus is fully standalone. + */ +int +virDomainStorageSourceFormatFull(virBufferPtr buf, + virStorageSourcePtr src, + const char *elemname, + bool status, + virDomainXMLOptionPtr xmlopt) +{ + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; + unsigned int flags = 0; + + if (status) + flags |= VIR_DOMAIN_DEF_FORMAT_STATUS; + + virBufferSetChildIndent(&childBuf, buf); + + virBufferAsprintf(&attrBuf, " type='%s' format='%s'", + virStorageTypeToString(src->type), + virStorageFileFormatTypeToString(src->format)); + + if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, + true, true, 0, xmlopt) < 0) + return -1; + + return virXMLFormatElement(buf, elemname, &attrBuf, &childBuf); +} + + static int virDomainDiskSourceFormatInternal(virBufferPtr buf, virStorageSourcePtr src, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a6f8e13088..547ce8c5b9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3467,6 +3467,14 @@ int virDomainStorageSourceFormat(virBufferPtr attrBuf, virDomainXMLOptionPtr xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int +virDomainStorageSourceFormatFull(virBufferPtr buf, + virStorageSourcePtr src, + const char *elemname, + bool status, + virDomainXMLOptionPtr xmlopt) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virStorageSourcePtr src, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9494a04bb..5b031ff877 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -564,6 +564,7 @@ virDomainStateTypeFromString; virDomainStateTypeToString; virDomainStorageNetworkParseHost; virDomainStorageSourceFormat; +virDomainStorageSourceFormatFull; virDomainStorageSourceParse; virDomainTaintTypeFromString; virDomainTaintTypeToString; -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:52PM +0100, Peter Krempa wrote:
The new function formats a virStorageSource into an XML element which already contains type and format for simpler handling.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 48 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4083839fc8..9f1f46c905 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23857,6 +23857,45 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, }
+/** + * virDomainStorageSourceFormatFull: + * @buf: output buffer + * @src: storage source to format + * @elemname: name of the top level element to use + * @status: output status-XML style private data + * @xmlopt: formatter callback data structure + * + * Formats @src into a XML element called @elemname. The element has both 'type' + * and 'format' attributes and thus is fully standalone. + */ +int +virDomainStorageSourceFormatFull(virBufferPtr buf, + virStorageSourcePtr src, + const char *elemname, + bool status, + virDomainXMLOptionPtr xmlopt) +{ + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; + unsigned int flags = 0; + + if (status) + flags |= VIR_DOMAIN_DEF_FORMAT_STATUS; + + virBufferSetChildIndent(&childBuf, buf); + + virBufferAsprintf(&attrBuf, " type='%s' format='%s'", + virStorageTypeToString(src->type), + virStorageFileFormatTypeToString(src->format)); + + if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, + true, true, 0, xmlopt) < 0)
Everything is formatted, including the index attribute.
+ return -1; + + return virXMLFormatElement(buf, elemname, &attrBuf, &childBuf); +} + + static int virDomainDiskSourceFormatInternal(virBufferPtr buf, virStorageSourcePtr src,
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use virDomainStorageSourceFormatFull which has the same functionality. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 43 ++++-------------------------------------- 1 file changed, 4 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d25d9ab666..78287b4d3e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2341,47 +2341,11 @@ qemuDomainObjPrivateXMLFormatPR(virBufferPtr buf, } -static int -qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, - virStorageSourcePtr src) -{ - VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; - VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; - VIR_AUTOCLEAN(virBuffer) privateDataBuf = VIR_BUFFER_INITIALIZER; - int ret = -1; - - virBufferSetChildIndent(&childBuf, buf); - virBufferSetChildIndent(&privateDataBuf, &childBuf); - - virBufferAsprintf(&attrBuf, " type='%s' format='%s'", - virStorageTypeToString(src->type), - virStorageFileFormatTypeToString(src->format)); - - if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, - VIR_DOMAIN_DEF_FORMAT_STATUS, true, - false, 0, NULL) < 0) - goto cleanup; - - if (qemuStorageSourcePrivateDataFormat(src, &privateDataBuf) < 0) - goto cleanup; - - if (virXMLFormatElement(&childBuf, "privateData", NULL, &privateDataBuf) < 0) - goto cleanup; - - if (virXMLFormatElement(buf, "migrationSource", &attrBuf, &childBuf) < 0) - goto cleanup; - - ret = 0; - - cleanup: - return ret; -} - - static int qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; size_t i; @@ -2399,8 +2363,9 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, disk->dst, diskPriv->migrating ? "yes" : "no"); if (diskPriv->migrSource && - qemuDomainObjPrivateXMLFormatNBDMigrationSource(&childBuf, - diskPriv->migrSource) < 0) + virDomainStorageSourceFormatFull(&childBuf, diskPriv->migrSource, + "migrationSource", true, + priv->driver->xmlopt) < 0) goto cleanup; if (virXMLFormatElement(buf, "disk", &attrBuf, &childBuf) < 0) -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:53PM +0100, Peter Krempa wrote:
Use virDomainStorageSourceFormatFull which has the same functionality.
Not quite,
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 43 ++++-------------------------------------- 1 file changed, 4 insertions(+), 39 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d25d9ab666..78287b4d3e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2341,47 +2341,11 @@ qemuDomainObjPrivateXMLFormatPR(virBufferPtr buf, }
-static int -qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, - virStorageSourcePtr src) -{ - VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; - VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; - VIR_AUTOCLEAN(virBuffer) privateDataBuf = VIR_BUFFER_INITIALIZER; - int ret = -1; - - virBufferSetChildIndent(&childBuf, buf); - virBufferSetChildIndent(&privateDataBuf, &childBuf); - - virBufferAsprintf(&attrBuf, " type='%s' format='%s'", - virStorageTypeToString(src->type), - virStorageFileFormatTypeToString(src->format)); - - if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, - VIR_DOMAIN_DEF_FORMAT_STATUS, true, - false, 0, NULL) < 0)
the index attribute was not formatted here. (It won't be formatted anyway because AFAIK it's always 0, but it's worth mentioning in the commit message) Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
It's not used outside of src/conf/domain_conf.c Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 10 ---------- src/libvirt_private.syms | 1 - 3 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9f1f46c905..32f6d88596 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23772,7 +23772,7 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, * Note that this does _not_ format the 'type' and 'format' of @src due to * differences in callers. */ -int +static int virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 547ce8c5b9..51e0757f5c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3457,16 +3457,6 @@ int virDomainDiskDefCheckDuplicateInfo(const virDomainDiskDef *a, const virDomainDiskDef *b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int virDomainStorageSourceFormat(virBufferPtr attrBuf, - virBufferPtr childBuf, - virStorageSourcePtr src, - unsigned int flags, - bool seclabels, - bool attrIndex, - int policy, - virDomainXMLOptionPtr xmlopt) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); - int virDomainStorageSourceFormatFull(virBufferPtr buf, virStorageSourcePtr src, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5b031ff877..010c8ac481 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -563,7 +563,6 @@ virDomainStateReasonToString; virDomainStateTypeFromString; virDomainStateTypeToString; virDomainStorageNetworkParseHost; -virDomainStorageSourceFormat; virDomainStorageSourceFormatFull; virDomainStorageSourceParse; virDomainTaintTypeFromString; -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:54PM +0100, Peter Krempa wrote:
It's not used outside of src/conf/domain_conf.c
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 10 ---------- src/libvirt_private.syms | 1 - 3 files changed, 1 insertion(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virDomainDiskSourceFormat would call virDomainDiskSourceFormatInternal with a limited set of parameters. Remove the 'Internal' variant by squishing into virDomainDiskSourceFormat and fix the callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 38 +++++++++++++------------------------- src/conf/domain_conf.h | 2 ++ src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- tests/virstoragetest.c | 2 +- 5 files changed, 18 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 32f6d88596..79e15c9886 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23896,14 +23896,14 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, } -static int -virDomainDiskSourceFormatInternal(virBufferPtr buf, - virStorageSourcePtr src, - int policy, - unsigned int flags, - bool seclabels, - bool attrIndex, - virDomainXMLOptionPtr xmlopt) +int +virDomainDiskSourceFormat(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + unsigned int flags, + bool seclabels, + bool attrIndex, + virDomainXMLOptionPtr xmlopt) { VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; @@ -23925,18 +23925,6 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, } -int -virDomainDiskSourceFormat(virBufferPtr buf, - virStorageSourcePtr src, - int policy, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - return virDomainDiskSourceFormatInternal(buf, src, policy, flags, true, - false, xmlopt); -} - - static int virDomainDiskBackingStoreFormat(virBufferPtr buf, virStorageSourcePtr backingStore, @@ -23975,8 +23963,8 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAsprintf(buf, "<format type='%s'/>\n", format); /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, false, - false, xmlopt) < 0 || + if (virDomainDiskSourceFormat(buf, backingStore, 0, flags, false, + false, xmlopt) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, xmlopt, flags) < 0) return -1; @@ -24137,7 +24125,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(buf, disk->mirror, 0, 0, xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, disk->mirror, 0, 0, true, false, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</mirror>\n"); @@ -24233,8 +24221,8 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->src->auth && !def->src->authInherited) virStorageAuthDefFormat(buf, def->src->auth); - if (virDomainDiskSourceFormatInternal(buf, def->src, def->startupPolicy, - flags, true, true, xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy, + flags, true, true, xmlopt) < 0) return -1; /* Don't format backingStore to inactive XMLs until the code for diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 51e0757f5c..7ea9822fe4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3016,6 +3016,8 @@ int virDomainDiskSourceFormat(virBufferPtr buf, virStorageSourcePtr src, int policy, unsigned int flags, + bool seclabels, + bool attrIndex, virDomainXMLOptionPtr xmlopt); int virDomainNetDefFormat(virBufferPtr buf, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index ffb1313c89..a849a58da3 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -768,7 +768,7 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (disk->src->format > 0) virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->src->format)); - if (virDomainDiskSourceFormat(buf, disk->src, 0, 0, xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, disk->src, 0, 0, true, false, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 4cd15a1dff..6b5571b7cb 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -87,7 +87,7 @@ testBackingXMLjsonXML(const void *args) goto cleanup; } - if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0, true, false, NULL) < 0 || !(actualxml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); goto cleanup; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index fb98903f02..75c60da537 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -657,7 +657,7 @@ testBackingParse(const void *args) goto cleanup; } - if (virDomainDiskSourceFormat(&buf, src, 0, 0, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, src, 0, 0, true, false, NULL) < 0 || !(xml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); goto cleanup; -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:55PM +0100, Peter Krempa wrote:
virDomainDiskSourceFormat would call virDomainDiskSourceFormatInternal with a limited set of parameters. Remove the 'Internal' variant by squishing into virDomainDiskSourceFormat and fix the callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 38 +++++++++++++------------------------- src/conf/domain_conf.h | 2 ++ src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- tests/virstoragetest.c | 2 +- 5 files changed, 18 insertions(+), 28 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Now that the cleanup section is handled automatically it can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 79e15c9886..b78481c280 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23907,21 +23907,14 @@ virDomainDiskSourceFormat(virBufferPtr buf, { VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; - int ret = -1; virBufferSetChildIndent(&childBuf, buf); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, seclabels, attrIndex, policy, xmlopt) < 0) - goto cleanup; - - if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return virXMLFormatElement(buf, "source", &attrBuf, &childBuf); } -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:56PM +0100, Peter Krempa wrote:
Now that the cleanup section is handled automatically it can be removed.
d/section /
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Modify the check that the format is in range to be standalone and use the convertor function directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b78481c280..ae17992b83 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23924,7 +23924,6 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virDomainXMLOptionPtr xmlopt, unsigned int flags) { - const char *format; bool inactive = flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE; if (!backingStore) @@ -23939,8 +23938,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, return 0; } - if (backingStore->format <= 0 || - !(format = virStorageFileFormatTypeToString(backingStore->format))) { + if (backingStore->format <= 0 || backingStore->format >= VIR_STORAGE_FILE_LAST) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk backing store format %d"), backingStore->format); @@ -23954,7 +23952,8 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<format type='%s'/>\n", format); + virBufferAsprintf(buf, "<format type='%s'/>\n", + virStorageFileFormatTypeToString(backingStore->format)); /* We currently don't output seclabels for backing chain element */ if (virDomainDiskSourceFormat(buf, backingStore, 0, flags, false, false, xmlopt) < 0 || -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:57PM +0100, Peter Krempa wrote:
Modify the check that the format is in range to be standalone and use the convertor function directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae17992b83..2b6a2b7ee8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23924,8 +23924,12 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virDomainXMLOptionPtr xmlopt, unsigned int flags) { + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; bool inactive = flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE; + virBufferSetChildIndent(&childBuf, buf); + if (!backingStore) return 0; @@ -23945,25 +23949,23 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, return -1; } - virBufferAsprintf(buf, "<backingStore type='%s'", + virBufferAsprintf(&attrBuf, " type='%s'", virStorageTypeToString(backingStore->type)); if (backingStore->id != 0) - virBufferAsprintf(buf, " index='%u'", backingStore->id); - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); + virBufferAsprintf(&attrBuf, " index='%u'", backingStore->id); - virBufferAsprintf(buf, "<format type='%s'/>\n", + virBufferAsprintf(&childBuf, "<format type='%s'/>\n", virStorageFileFormatTypeToString(backingStore->format)); /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormat(buf, backingStore, 0, flags, false, - false, xmlopt) < 0 || - virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, + if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, false, + false, xmlopt) < 0) + return -1; + + if (virDomainDiskBackingStoreFormat(&childBuf, backingStore->backingStore, xmlopt, flags) < 0) return -1; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</backingStore>\n"); - return 0; + return virXMLFormatElement(buf, "backingStore", &attrBuf, &childBuf); } -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:58PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Upcoming commits will reorganize control flow leading to calling the function to virDomainDiskSourceFormat thus it needs to be moved slightly up. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 102 ++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2b6a2b7ee8..9d738f06cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23857,6 +23857,57 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, } +static int +virDomainDiskBackingStoreFormat(virBufferPtr buf, + virStorageSourcePtr backingStore, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; + bool inactive = flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE; + + virBufferSetChildIndent(&childBuf, buf); + + if (!backingStore) + return 0; + + /* don't write detected backing chain members to inactive xml */ + if (inactive && backingStore->detected) + return 0; + + if (backingStore->type == VIR_STORAGE_TYPE_NONE) { + virBufferAddLit(buf, "<backingStore/>\n"); + return 0; + } + + if (backingStore->format <= 0 || backingStore->format >= VIR_STORAGE_FILE_LAST) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store format %d"), + backingStore->format); + return -1; + } + + virBufferAsprintf(&attrBuf, " type='%s'", + virStorageTypeToString(backingStore->type)); + if (backingStore->id != 0) + virBufferAsprintf(&attrBuf, " index='%u'", backingStore->id); + + virBufferAsprintf(&childBuf, "<format type='%s'/>\n", + virStorageFileFormatTypeToString(backingStore->format)); + /* We currently don't output seclabels for backing chain element */ + if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, false, + false, xmlopt) < 0) + return -1; + + if (virDomainDiskBackingStoreFormat(&childBuf, backingStore->backingStore, + xmlopt, flags) < 0) + return -1; + + return virXMLFormatElement(buf, "backingStore", &attrBuf, &childBuf); +} + + /** * virDomainStorageSourceFormatFull: * @buf: output buffer @@ -23918,57 +23969,6 @@ virDomainDiskSourceFormat(virBufferPtr buf, } -static int -virDomainDiskBackingStoreFormat(virBufferPtr buf, - virStorageSourcePtr backingStore, - virDomainXMLOptionPtr xmlopt, - unsigned int flags) -{ - VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; - VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; - bool inactive = flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE; - - virBufferSetChildIndent(&childBuf, buf); - - if (!backingStore) - return 0; - - /* don't write detected backing chain members to inactive xml */ - if (inactive && backingStore->detected) - return 0; - - if (backingStore->type == VIR_STORAGE_TYPE_NONE) { - virBufferAddLit(buf, "<backingStore/>\n"); - return 0; - } - - if (backingStore->format <= 0 || backingStore->format >= VIR_STORAGE_FILE_LAST) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk backing store format %d"), - backingStore->format); - return -1; - } - - virBufferAsprintf(&attrBuf, " type='%s'", - virStorageTypeToString(backingStore->type)); - if (backingStore->id != 0) - virBufferAsprintf(&attrBuf, " index='%u'", backingStore->id); - - virBufferAsprintf(&childBuf, "<format type='%s'/>\n", - virStorageFileFormatTypeToString(backingStore->format)); - /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, false, - false, xmlopt) < 0) - return -1; - - if (virDomainDiskBackingStoreFormat(&childBuf, backingStore->backingStore, - xmlopt, flags) < 0) - return -1; - - return virXMLFormatElement(buf, "backingStore", &attrBuf, &childBuf); -} - - #define FORMAT_IOTUNE(val) \ if (disk->blkdeviotune.val) { \ virBufferAsprintf(&childBuf, "<" #val ">%llu</" #val ">\n", \ -- 2.20.1
On Mon, Mar 18, 2019 at 04:54:59PM +0100, Peter Krempa wrote:
Upcoming commits will reorganize control flow leading to calling the function to virDomainDiskSourceFormat thus it needs to be moved slightly up.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 102 ++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 51 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move the recursion to format the full backing store into virDomainDiskSourceFormat controlled by a new argument so that it's simpler to reuse in other places. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 30 +++++++++++++++--------------- src/conf/domain_conf.h | 1 + src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- tests/virstoragetest.c | 2 +- 5 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9d738f06cd..b67f9bbd2c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23897,11 +23897,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virStorageFileFormatTypeToString(backingStore->format)); /* We currently don't output seclabels for backing chain element */ if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, false, - false, xmlopt) < 0) - return -1; - - if (virDomainDiskBackingStoreFormat(&childBuf, backingStore->backingStore, - xmlopt, flags) < 0) + false, true, xmlopt) < 0) return -1; return virXMLFormatElement(buf, "backingStore", &attrBuf, &childBuf); @@ -23954,6 +23950,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, unsigned int flags, bool seclabels, bool attrIndex, + bool backingStore, virDomainXMLOptionPtr xmlopt) { VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; @@ -23962,10 +23959,18 @@ virDomainDiskSourceFormat(virBufferPtr buf, virBufferSetChildIndent(&childBuf, buf); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - seclabels, attrIndex, policy, xmlopt) < 0) + seclabels, attrIndex, + policy, xmlopt) < 0) return -1; - return virXMLFormatElement(buf, "source", &attrBuf, &childBuf); + if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) + return -1; + + if (backingStore && src->backingStore && + virDomainDiskBackingStoreFormat(buf, src->backingStore, xmlopt, flags) < 0) + return -1; + + return 0; } @@ -24119,7 +24124,8 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(buf, disk->mirror, 0, 0, true, false, xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, disk->mirror, 0, 0, true, false, false, + xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</mirror>\n"); @@ -24216,13 +24222,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virStorageAuthDefFormat(buf, def->src->auth); if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy, - flags, true, true, xmlopt) < 0) - return -1; - - /* Don't format backingStore to inactive XMLs until the code for - * persistent storage of backing chains is ready. */ - if (virDomainDiskBackingStoreFormat(buf, def->src->backingStore, - xmlopt, flags) < 0) + flags, true, true, true, xmlopt) < 0) return -1; virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7ea9822fe4..b373dbf939 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3018,6 +3018,7 @@ int virDomainDiskSourceFormat(virBufferPtr buf, unsigned int flags, bool seclabels, bool attrIndex, + bool backingStore, virDomainXMLOptionPtr xmlopt); int virDomainNetDefFormat(virBufferPtr buf, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index a849a58da3..bc4b9c8f11 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -768,7 +768,7 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (disk->src->format > 0) virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->src->format)); - if (virDomainDiskSourceFormat(buf, disk->src, 0, 0, true, false, xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, disk->src, 0, 0, true, false, false, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 6b5571b7cb..f40cba36cd 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -87,7 +87,7 @@ testBackingXMLjsonXML(const void *args) goto cleanup; } - if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0, true, false, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0, true, false, false, NULL) < 0 || !(actualxml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); goto cleanup; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 75c60da537..9a11f5bfe8 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -657,7 +657,7 @@ testBackingParse(const void *args) goto cleanup; } - if (virDomainDiskSourceFormat(&buf, src, 0, 0, true, false, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, src, 0, 0, true, false, false, NULL) < 0 || !(xml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); goto cleanup; -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:00PM +0100, Peter Krempa wrote:
Move the recursion to format the full backing store into virDomainDiskSourceFormat controlled by a new argument so that it's simpler to reuse in other places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 30 +++++++++++++++--------------- src/conf/domain_conf.h | 1 + src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- tests/virstoragetest.c | 2 +- 5 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9d738f06cd..b67f9bbd2c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23954,6 +23950,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, unsigned int flags, bool seclabels, bool attrIndex, + bool backingStore,
Eww, third bool in a row.
virDomainXMLOptionPtr xmlopt) { VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; @@ -23962,10 +23959,18 @@ virDomainDiskSourceFormat(virBufferPtr buf, virBufferSetChildIndent(&childBuf, buf);
if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - seclabels, attrIndex, policy, xmlopt) < 0) + seclabels, attrIndex, + policy, xmlopt) < 0)
Unrelated whitespace change.
return -1;
- return virXMLFormatElement(buf, "source", &attrBuf, &childBuf); + if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) + return -1;
You changed this just a few commits ago.
+ + if (backingStore && src->backingStore &&
Will the src->backingStore condition be necessary here? virDomainDiskBackingStoreFormat currently returns 0 for NULL source.
+ virDomainDiskBackingStoreFormat(buf, src->backingStore, xmlopt, flags) < 0) + return -1; + + return 0; }
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add switch which will allow formating full chain. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 11 ++++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b67f9bbd2c..fb98629c77 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23910,6 +23910,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, * @src: storage source to format * @elemname: name of the top level element to use * @status: output status-XML style private data + * @backingStore: output full backing chain of @src * @xmlopt: formatter callback data structure * * Formats @src into a XML element called @elemname. The element has both 'type' @@ -23920,6 +23921,7 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, virStorageSourcePtr src, const char *elemname, bool status, + bool backingStore, virDomainXMLOptionPtr xmlopt) { VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; @@ -23939,7 +23941,14 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, true, true, 0, xmlopt) < 0) return -1; - return virXMLFormatElement(buf, elemname, &attrBuf, &childBuf); + if (virXMLFormatElement(buf, elemname, &attrBuf, &childBuf) < 0) + return -1; + + if (backingStore && src->backingStore && + virDomainDiskBackingStoreFormat(buf, src->backingStore, xmlopt, flags) < 0) + return -1; + + return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b373dbf939..0bdd6c0f55 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3465,6 +3465,7 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, virStorageSourcePtr src, const char *elemname, bool status, + bool backingStore, virDomainXMLOptionPtr xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 78287b4d3e..ea70979f8b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2364,7 +2364,7 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, if (diskPriv->migrSource && virDomainStorageSourceFormatFull(&childBuf, diskPriv->migrSource, - "migrationSource", true, + "migrationSource", true, false, priv->driver->xmlopt) < 0) goto cleanup; -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:01PM +0100, Peter Krempa wrote:
Add switch which will allow formating full chain.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 11 ++++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b67f9bbd2c..fb98629c77 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23910,6 +23910,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, * @src: storage source to format * @elemname: name of the top level element to use * @status: output status-XML style private data + * @backingStore: output full backing chain of @src * @xmlopt: formatter callback data structure * * Formats @src into a XML element called @elemname. The element has both 'type' @@ -23920,6 +23921,7 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, virStorageSourcePtr src, const char *elemname, bool status, + bool backingStore, virDomainXMLOptionPtr xmlopt) { VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; @@ -23939,7 +23941,14 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, true, true, 0, xmlopt) < 0) return -1;
- return virXMLFormatElement(buf, elemname, &attrBuf, &childBuf); + if (virXMLFormatElement(buf, elemname, &attrBuf, &childBuf) < 0) + return -1; + + if (backingStore && src->backingStore &&
Same commend about the condition.
+ virDomainDiskBackingStoreFormat(buf, src->backingStore, xmlopt, flags) < 0) + return -1; + + return 0;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virDomainDiskSourcePrivateDataParse and virDomainDiskSourcePRParse don't need the 'cleanup' label any more thanks to VIR_XPATH_NODE_AUTORESTORE. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fb98629c77..0c650ffc2c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9028,7 +9028,6 @@ virDomainDiskSourcePrivateDataParse(xmlNodePtr node, virDomainXMLOptionPtr xmlopt) { VIR_XPATH_NODE_AUTORESTORE(ctxt); - int ret = -1; if (!(flags & VIR_DOMAIN_DEF_PARSE_STATUS) || !xmlopt || !xmlopt->privateData.storageParse) @@ -9036,18 +9035,13 @@ virDomainDiskSourcePrivateDataParse(xmlNodePtr node, ctxt->node = node; - if (!(ctxt->node = virXPathNode("./privateData", ctxt))) { - ret = 0; - goto cleanup; - } + if (!(ctxt->node = virXPathNode("./privateData", ctxt))) + return 0; if (xmlopt->privateData.storageParse(ctxt, src) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } @@ -9057,21 +9051,16 @@ virDomainDiskSourcePRParse(xmlNodePtr node, virStoragePRDefPtr *pr) { VIR_XPATH_NODE_AUTORESTORE(ctxt); - int ret = -1; ctxt->node = node; - if (!(ctxt->node = virXPathNode("./reservations", ctxt))) { - ret = 0; - goto cleanup; - } + if (!(ctxt->node = virXPathNode("./reservations", ctxt))) + return 0; if (!(*pr = virStoragePRDefParseXML(ctxt))) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:02PM +0100, Peter Krempa wrote:
virDomainDiskSourcePrivateDataParse and virDomainDiskSourcePRParse don't need the 'cleanup' label any more thanks to VIR_XPATH_NODE_AUTORESTORE.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
--- src/conf/domain_conf.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0c650ffc2c..ef164f3375 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9070,7 +9070,6 @@ virDomainStorageSourceParse(xmlNodePtr node, virStorageSourcePtr src, unsigned int flags) { - int ret = -1; VIR_XPATH_NODE_AUTORESTORE(ctxt); xmlNodePtr tmp; @@ -9088,34 +9087,34 @@ virDomainStorageSourceParse(xmlNodePtr node, break; case VIR_STORAGE_TYPE_NETWORK: if (virDomainDiskSourceNetworkParse(node, ctxt, src, flags) < 0) - goto cleanup; + return -1; break; case VIR_STORAGE_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) - goto cleanup; + return -1; break; case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %s"), virStorageTypeToString(src->type)); - goto cleanup; + return -1; } if ((tmp = virXPathNode("./auth", ctxt)) && !(src->auth = virStorageAuthDefParse(tmp, ctxt))) - goto cleanup; + return -1; if ((tmp = virXPathNode("./encryption", ctxt)) && !(src->encryption = virStorageEncryptionParseNode(tmp, ctxt))) - goto cleanup; + return -1; if (virDomainDiskSourcePRParse(node, ctxt, &src->pr) < 0) - goto cleanup; + return -1; if (virSecurityDeviceLabelDefParseXML(&src->seclabels, &src->nseclabels, ctxt, flags) < 0) - goto cleanup; + return -1; /* People sometimes pass a bogus '' source path when they mean to omit the * source element completely (e.g. CDROM without media). This is just a @@ -9123,10 +9122,7 @@ virDomainStorageSourceParse(xmlNodePtr node, if (src->path && !*src->path) VIR_FREE(src->path); - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:03PM +0100, Peter Krempa wrote:
--- src/conf/domain_conf.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We can use our VIR_AUTOPTR machinery also for libxml2's xmlDoc and xmlXPathContext. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virxml.h b/src/util/virxml.h index b91fedde82..4875e88f2e 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -244,4 +244,7 @@ VIR_DEFINE_AUTOCLEAN_FUNC(virXPathContextNodeSave, virXPathContextNodeRestore); .node = _ctxt->node}; \ ignore_value(&_ctxt ## CtxtSave) +VIR_DEFINE_AUTOPTR_FUNC(xmlDoc, xmlFreeDoc); +VIR_DEFINE_AUTOPTR_FUNC(xmlXPathContext, xmlXPathFreeContext); + #endif /* LIBVIRT_VIRXML_H */ -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:04PM +0100, Peter Krempa wrote:
We can use our VIR_AUTOPTR machinery also for libxml2's xmlDoc and xmlXPathContext.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.h | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index f40cba36cd..b1d1ed943c 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -43,14 +43,14 @@ static int testBackingXMLjsonXML(const void *args) { const struct testBackingXMLjsonXMLdata *data = args; - xmlDocPtr xml = NULL; - xmlXPathContextPtr ctxt = NULL; - virBuffer buf = VIR_BUFFER_INITIALIZER; - virJSONValuePtr backendprops = NULL; - virJSONValuePtr wrapper = NULL; - char *propsstr = NULL; - char *protocolwrapper = NULL; - char *actualxml = NULL; + VIR_AUTOPTR(xmlDoc) xml = NULL; + VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; + VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; + VIR_AUTOPTR(virJSONValue) backendprops = NULL; + VIR_AUTOPTR(virJSONValue) wrapper = NULL; + VIR_AUTOFREE(char *) propsstr = NULL; + VIR_AUTOFREE(char *) protocolwrapper = NULL; + VIR_AUTOFREE(char *) actualxml = NULL; int ret = -1; VIR_AUTOUNREF(virStorageSourcePtr) xmlsrc = NULL; VIR_AUTOUNREF(virStorageSourcePtr) jsonsrc = NULL; @@ -104,15 +104,6 @@ testBackingXMLjsonXML(const void *args) ret = 0; cleanup: - VIR_FREE(propsstr); - VIR_FREE(protocolwrapper); - VIR_FREE(actualxml); - virJSONValueFree(backendprops); - virJSONValueFree(wrapper); - virBufferFreeAndReset(&buf); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); - return ret; } -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:05PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Get rid of the 'cleanup' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index b1d1ed943c..96c70e381a 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -51,7 +51,6 @@ testBackingXMLjsonXML(const void *args) VIR_AUTOFREE(char *) propsstr = NULL; VIR_AUTOFREE(char *) protocolwrapper = NULL; VIR_AUTOFREE(char *) actualxml = NULL; - int ret = -1; VIR_AUTOUNREF(virStorageSourcePtr) xmlsrc = NULL; VIR_AUTOUNREF(virStorageSourcePtr) jsonsrc = NULL; @@ -61,36 +60,36 @@ testBackingXMLjsonXML(const void *args) xmlsrc->type = data->type; if (!(xml = virXMLParseStringCtxt(data->xml, "(test storage source XML)", &ctxt))) - goto cleanup; + return -1; if (virDomainDiskSourceParse(ctxt->node, ctxt, xmlsrc, 0, NULL) < 0) { fprintf(stderr, "failed to parse disk source xml\n"); - goto cleanup; + return -1; } if (!(backendprops = qemuBlockStorageSourceGetBackendProps(xmlsrc, true))) { fprintf(stderr, "failed to format disk source json\n"); - goto cleanup; + return -1; } if (virJSONValueObjectCreate(&wrapper, "a:file", &backendprops, NULL) < 0) - goto cleanup; + return -1; if (!(propsstr = virJSONValueToString(wrapper, false))) - goto cleanup; + return -1; if (virAsprintf(&protocolwrapper, "json:%s", propsstr) < 0) - goto cleanup; + return -1; if (!(jsonsrc = virStorageSourceNewFromBackingAbsolute(protocolwrapper))) { fprintf(stderr, "failed to parse disk json\n"); - goto cleanup; + return -1; } if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0, true, false, false, NULL) < 0 || !(actualxml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); - goto cleanup; + return -1; } if (STRNEQ(actualxml, data->xml)) { @@ -98,13 +97,10 @@ testBackingXMLjsonXML(const void *args) "actual storage source xml:\n%s\n" "intermediate json:\n%s\n", data->xml, actualxml, protocolwrapper); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:06PM +0100, Peter Krempa wrote:
Get rid of the 'cleanup' label.
Bye, cleanup label!
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function does not have any code in the 'cleanup' label so we can simplify the control flow. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ef164f3375..d74ac2f1fb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9151,19 +9151,16 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, { VIR_XPATH_NODE_AUTORESTORE(ctxt); xmlNodePtr source; - int ret = -1; VIR_AUTOUNREF(virStorageSourcePtr) backingStore = NULL; VIR_AUTOFREE(char *) type = NULL; VIR_AUTOFREE(char *) format = NULL; VIR_AUTOFREE(char *) idx = NULL; - if (!(ctxt->node = virXPathNode("./backingStore", ctxt))) { - ret = 0; - goto cleanup; - } + if (!(ctxt->node = virXPathNode("./backingStore", ctxt))) + return 0; if (!(backingStore = virStorageSourceNew())) - goto cleanup; + return -1; /* backing store is always read-only */ backingStore->readonly = true; @@ -9171,52 +9168,49 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, /* terminator does not have a type */ if (!(type = virXMLPropString(ctxt->node, "type"))) { VIR_STEAL_PTR(src->backingStore, backingStore); - ret = 0; - goto cleanup; + return 0; } if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && (idx = virXMLPropString(ctxt->node, "index")) && virStrToLong_uip(idx, NULL, 10, &backingStore->id) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid disk index '%s'"), idx); - goto cleanup; + return -1; } backingStore->type = virStorageTypeFromString(type); if (backingStore->type <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk backing store type '%s'"), type); - goto cleanup; + return -1; } if (!(format = virXPathString("string(./format/@type)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing disk backing store format")); - goto cleanup; + return -1; } backingStore->format = virStorageFileFormatTypeFromString(format); if (backingStore->format <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk backing store format '%s'"), format); - goto cleanup; + return -1; } if (!(source = virXPathNode("./source", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing disk backing store source")); - goto cleanup; + return -1; } if (virDomainDiskSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) - goto cleanup; + return -1; VIR_STEAL_PTR(src->backingStore, backingStore); - ret = 0; - cleanup: - return ret; + return 0; } #define PARSE_IOTUNE(val) \ -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:07PM +0100, Peter Krempa wrote:
The function does not have any code in the 'cleanup' label so we can simplify the control flow.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Storage source private data can be parsed along with other components of private data rather than a separate function which is called from multiple places. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 42 ++++++++++++------------------------------ src/conf/domain_conf.h | 3 ++- src/qemu/qemu_domain.c | 2 +- 3 files changed, 15 insertions(+), 32 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d74ac2f1fb..bf3ad45397 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9020,31 +9020,6 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, } -static int -virDomainDiskSourcePrivateDataParse(xmlNodePtr node, - xmlXPathContextPtr ctxt, - virStorageSourcePtr src, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - VIR_XPATH_NODE_AUTORESTORE(ctxt); - - if (!(flags & VIR_DOMAIN_DEF_PARSE_STATUS) || - !xmlopt || !xmlopt->privateData.storageParse) - return 0; - - ctxt->node = node; - - if (!(ctxt->node = virXPathNode("./privateData", ctxt))) - return 0; - - if (xmlopt->privateData.storageParse(ctxt, src) < 0) - return -1; - - return 0; -} - - static int virDomainDiskSourcePRParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -9068,7 +9043,8 @@ int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virStorageSourcePtr src, - unsigned int flags) + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { VIR_XPATH_NODE_AUTORESTORE(ctxt); xmlNodePtr tmp; @@ -9122,6 +9098,15 @@ virDomainStorageSourceParse(xmlNodePtr node, if (src->path && !*src->path) VIR_FREE(src->path); + if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) && + xmlopt && xmlopt->privateData.storageParse && + (tmp = virXPathNode("./privateData", ctxt))) { + ctxt->node = tmp; + + if (xmlopt->privateData.storageParse(ctxt, src) < 0) + return -1; + } + return 0; } @@ -9133,10 +9118,7 @@ virDomainDiskSourceParse(xmlNodePtr node, unsigned int flags, virDomainXMLOptionPtr xmlopt) { - if (virDomainStorageSourceParse(node, ctxt, src, flags) < 0) - return -1; - - if (virDomainDiskSourcePrivateDataParse(node, ctxt, src, flags, xmlopt) < 0) + if (virDomainStorageSourceParse(node, ctxt, src, flags, xmlopt) < 0) return -1; return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0bdd6c0f55..0b6d432871 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3472,7 +3472,8 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virStorageSourcePtr src, - unsigned int flags) + unsigned int flags, + virDomainXMLOptionPtr xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ea70979f8b..0a0a464dd2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2726,7 +2726,7 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, } if (virDomainStorageSourceParse(ctxt->node, ctxt, migrSource, - VIR_DOMAIN_DEF_PARSE_STATUS) < 0) + VIR_DOMAIN_DEF_PARSE_STATUS, NULL) < 0) goto cleanup; if ((ctxt->node = virXPathNode("./privateData", ctxt)) && -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:08PM +0100, Peter Krempa wrote:
Storage source private data can be parsed along with other components of private data rather than a separate function which is called from multiple places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 42 ++++++++++++------------------------------ src/conf/domain_conf.h | 3 ++- src/qemu/qemu_domain.c | 2 +- 3 files changed, 15 insertions(+), 32 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Introduce a helper which parses XML into virStorageSource according to XPath queries passed in for the various bits. The new parser will allow to unify the parsing of the various XML formats we use in different parts without the need to do custom parsers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 94 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 12 +++++ src/libvirt_private.syms | 1 + 3 files changed, 107 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bf3ad45397..d2cc199ed5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9111,6 +9111,100 @@ virDomainStorageSourceParse(xmlNodePtr node, } +/** + * virDomainStorageSourceParseFull + * @typeXPath: XPath query string for the 'type' of virStorageSource + * @formatXPath: XPath query for the image format (may be NULL if caller wishes to parse it) + * @sourceXPath: XPath query for the <source> subelement + * @indexXPath: XPath query for 'id' in virStorageSource (may be NULL if skipped) + * @allowMissing: if true no errors are reported if the above fields are missing + * @ctxt: XPath context + * @flags: XML parser flags + * @xmlopt: XML parser callbacks + * + * Uses the XPath queries provided as arguments to parse a storage source XML + * into virStorageSource. If allowMissing is false the function reports error if + * any of the XML parts described by @typeXPath, @formatXPath or @sourceXPath + * are missing. @formatXPath and @indexXpath may be NULL if they should be omitted + * or if the caller parses the value separately. + * + * Returns the parsed source or NULL on error. + */ +virStorageSourcePtr +virDomainStorageSourceParseFull(const char *typeXPath, + const char *formatXPath, + const char *sourceXPath, + const char *indexXPath, + bool allowMissing, + xmlXPathContextPtr ctxt, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) +{ + VIR_XPATH_NODE_AUTORESTORE(ctxt); + VIR_AUTOFREE(char *) type = NULL; + VIR_AUTOFREE(char *) format = NULL; + VIR_AUTOFREE(char *) idx = NULL; + xmlNodePtr sourceNode = NULL; + VIR_AUTOUNREF(virStorageSourcePtr) src = NULL; + virStorageSourcePtr ret = NULL; + + if (!(type = virXPathString(typeXPath, ctxt)) && + !allowMissing) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage source type")); + return NULL; + } + + if (formatXPath && + !(format = virXPathString(formatXPath, ctxt)) && + !allowMissing) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage source format")); + return NULL; + } + + if (!(sourceNode = virXPathNode(sourceXPath, ctxt)) && + !allowMissing) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage source data")); + return NULL; + } + + if (!(src = virStorageSourceNew())) + return NULL; + + if (type) { + if ((src->type = virStorageTypeFromString(type)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown storage source type '%s'"), type); + return NULL; + } + } else { + src->type = VIR_STORAGE_TYPE_FILE; + } + + if (format && + (src->format = virStorageFileFormatTypeFromString(format)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown storage source format '%s'"), format); + return NULL; + } + + if (indexXPath && + !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + (idx = virXPathString(indexXPath, ctxt)) && + virStrToLong_uip(idx, NULL, 10, &src->id) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid storage source index '%s'"), idx); + return NULL; + } + + if (sourceNode && + virDomainStorageSourceParse(sourceNode, ctxt, src, flags, xmlopt) < 0) + return NULL; + + VIR_STEAL_PTR(ret, src); + return ret; +} + + int virDomainDiskSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0b6d432871..a87e1b30b9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3476,6 +3476,18 @@ int virDomainStorageSourceParse(xmlNodePtr node, virDomainXMLOptionPtr xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +virStorageSourcePtr +virDomainStorageSourceParseFull(const char *typeXPath, + const char *formatXPath, + const char *sourceXPath, + const char *indexXPath, + bool allowMissing, + xmlXPathContextPtr ctxt, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(6); + int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, int maplen, int ncpumaps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 010c8ac481..e4a695de9f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -565,6 +565,7 @@ virDomainStateTypeToString; virDomainStorageNetworkParseHost; virDomainStorageSourceFormatFull; virDomainStorageSourceParse; +virDomainStorageSourceParseFull; virDomainTaintTypeFromString; virDomainTaintTypeToString; virDomainTimerModeTypeFromString; -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:09PM +0100, Peter Krempa wrote:
Introduce a helper which parses XML into virStorageSource according to XPath queries passed in for the various bits. The new parser will allow to unify the parsing of the various XML formats we use in different parts without the need to do custom parsers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 94 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 12 +++++ src/libvirt_private.syms | 1 + 3 files changed, 107 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bf3ad45397..d2cc199ed5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9111,6 +9111,100 @@ virDomainStorageSourceParse(xmlNodePtr node, }
+/** + * virDomainStorageSourceParseFull + * @typeXPath: XPath query string for the 'type' of virStorageSource
Does this need to be an XPath query as opposed to a simple attribute queried by virXMLPropString? All the callers pass either "string(@type)" "or string(./@type)"
+ * @formatXPath: XPath query for the image format (may be NULL if caller wishes to parse it) + * @sourceXPath: XPath query for the <source> subelement + * @indexXPath: XPath query for 'id' in virStorageSource (may be NULL if skipped) + * @allowMissing: if true no errors are reported if the above fields are missing + * @ctxt: XPath context + * @flags: XML parser flags + * @xmlopt: XML parser callbacks + *
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The new helper allows to avoid hand-rolled code to parse a storage source. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 62 ++++++++---------------------------------- 1 file changed, 12 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0a0a464dd2..e406647d8f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2681,65 +2681,27 @@ qemuDomainObjPrivateXMLParsePR(xmlXPathContextPtr ctxt, static int qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, + qemuDomainObjPrivatePtr priv, xmlXPathContextPtr ctxt, virDomainDiskDefPtr disk) { VIR_XPATH_NODE_AUTORESTORE(ctxt); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - char *format = NULL; - char *type = NULL; - int ret = -1; - VIR_AUTOUNREF(virStorageSourcePtr) migrSource = NULL; ctxt->node = node; - if (!(ctxt->node = virXPathNode("./migrationSource", ctxt))) { - ret = 0; - goto cleanup; - } - - if (!(migrSource = virStorageSourceNew())) - goto cleanup; - - if (!(type = virXMLPropString(ctxt->node, "type"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage source type")); - goto cleanup; - } - - if (!(format = virXMLPropString(ctxt->node, "format"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage source format")); - goto cleanup; - } - - if ((migrSource->type = virStorageTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage source type '%s'"), type); - goto cleanup; - } - - if ((migrSource->format = virStorageFileFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage source format '%s'"), format); - goto cleanup; - } - - if (virDomainStorageSourceParse(ctxt->node, ctxt, migrSource, - VIR_DOMAIN_DEF_PARSE_STATUS, NULL) < 0) - goto cleanup; - - if ((ctxt->node = virXPathNode("./privateData", ctxt)) && - qemuStorageSourcePrivateDataParse(ctxt, migrSource) < 0) - goto cleanup; + if (!(ctxt->node = virXPathNode("./migrationSource", ctxt))) + return 0; - VIR_STEAL_PTR(diskPriv->migrSource, migrSource); - ret = 0; + if (!(diskPriv->migrSource = virDomainStorageSourceParseFull("string(@type)", + "string(@format)", + ".", NULL, + false, ctxt, + VIR_DOMAIN_DEF_PARSE_STATUS, + priv->driver->xmlopt))) + return -1; - cleanup: - VIR_FREE(format); - VIR_FREE(type); - return ret; + return 0; } @@ -2770,7 +2732,7 @@ qemuDomainObjPrivateXMLParseJobNBD(virDomainObjPtr vm, (disk = virDomainDiskByName(vm->def, dst, false))) { QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating = true; - if (qemuDomainObjPrivateXMLParseJobNBDSource(nodes[i], ctxt, + if (qemuDomainObjPrivateXMLParseJobNBDSource(nodes[i], priv, ctxt, disk) < 0) goto cleanup; } -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:10PM +0100, Peter Krempa wrote:
The new helper allows to avoid hand-rolled code to parse a storage source.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 62 ++++++++---------------------------------- 1 file changed, 12 insertions(+), 50 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Now that all external callers were changed we can stop exporting this func. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 7 ------- src/libvirt_private.syms | 1 - 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d2cc199ed5..e7f3bcf114 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9039,7 +9039,7 @@ virDomainDiskSourcePRParse(xmlNodePtr node, } -int +static int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virStorageSourcePtr src, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a87e1b30b9..ad288d702b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3469,13 +3469,6 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, virDomainXMLOptionPtr xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); -int virDomainStorageSourceParse(xmlNodePtr node, - xmlXPathContextPtr ctxt, - virStorageSourcePtr src, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); - virStorageSourcePtr virDomainStorageSourceParseFull(const char *typeXPath, const char *formatXPath, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e4a695de9f..4bdea3b58f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -564,7 +564,6 @@ virDomainStateTypeFromString; virDomainStateTypeToString; virDomainStorageNetworkParseHost; virDomainStorageSourceFormatFull; -virDomainStorageSourceParse; virDomainStorageSourceParseFull; virDomainTaintTypeFromString; virDomainTaintTypeToString; -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:11PM +0100, Peter Krempa wrote:
Now that all external callers were changed we can stop exporting this func.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 7 ------- src/libvirt_private.syms | 1 - 3 files changed, 1 insertion(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This is part of the effort to minimize use of virDomainDiskSourceParse. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 130 ++++++++++++++++++++---------------------- 1 file changed, 61 insertions(+), 69 deletions(-) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 96c70e381a..48cec2869b 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -34,15 +34,10 @@ VIR_LOG_INIT("tests.storagetest"); -struct testBackingXMLjsonXMLdata { - int type; - const char *xml; -}; - static int testBackingXMLjsonXML(const void *args) { - const struct testBackingXMLjsonXMLdata *data = args; + const char *xmlstr = args; VIR_AUTOPTR(xmlDoc) xml = NULL; VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -57,12 +52,13 @@ testBackingXMLjsonXML(const void *args) if (!(xmlsrc = virStorageSourceNew())) return -1; - xmlsrc->type = data->type; - - if (!(xml = virXMLParseStringCtxt(data->xml, "(test storage source XML)", &ctxt))) + if (!(xml = virXMLParseStringCtxt(xmlstr, "(test storage source XML)", &ctxt))) return -1; - if (virDomainDiskSourceParse(ctxt->node, ctxt, xmlsrc, 0, NULL) < 0) { + if (!(xmlsrc = virDomainStorageSourceParseFull("string(./@type)", + "string(./@format)", + ".", NULL, false, + ctxt, 0, NULL))) { fprintf(stderr, "failed to parse disk source xml\n"); return -1; } @@ -86,17 +82,19 @@ testBackingXMLjsonXML(const void *args) return -1; } - if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0, true, false, false, NULL) < 0 || + jsonsrc->format = VIR_STORAGE_FILE_RAW; + + if (virDomainStorageSourceFormatFull(&buf, jsonsrc, "source", false, false, NULL) < 0 || !(actualxml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); return -1; } - if (STRNEQ(actualxml, data->xml)) { + if (STRNEQ(actualxml, xmlstr)) { fprintf(stderr, "\n expected storage source xml:\n'%s'\n" "actual storage source xml:\n%s\n" "intermediate json:\n%s\n", - data->xml, actualxml, protocolwrapper); + xmlstr, actualxml, protocolwrapper); return -1; } @@ -317,7 +315,6 @@ mymain(void) { int ret = 0; virQEMUDriver driver; - struct testBackingXMLjsonXMLdata xmljsonxmldata; struct testQemuDiskXMLToJSONData diskxmljsondata; char *capslatest_x86_64 = NULL; virQEMUCapsPtr caps_x86_64 = NULL; @@ -341,70 +338,65 @@ mymain(void) virTestCounterReset("qemu storage source xml->json->xml "); -# define TEST_JSON_FORMAT(tpe, xmlstr) \ +# define TEST_JSON_FORMAT(xmlstr) \ do { \ - xmljsonxmldata.type = tpe; \ - xmljsonxmldata.xml = xmlstr; \ if (virTestRun(virTestCounterNext(), testBackingXMLjsonXML, \ - &xmljsonxmldata) < 0) \ + xmlstr) < 0) \ ret = -1; \ } while (0) -# define TEST_JSON_FORMAT_NET(xmlstr) \ - TEST_JSON_FORMAT(VIR_STORAGE_TYPE_NETWORK, xmlstr) - - TEST_JSON_FORMAT(VIR_STORAGE_TYPE_FILE, "<source file='/path/to/file'/>\n"); + TEST_JSON_FORMAT("<source type='file' format='raw' file='/path/to/file'/>\n"); /* type VIR_STORAGE_TYPE_BLOCK is not tested since it parses back to 'file' */ /* type VIR_STORAGE_TYPE_DIR it is a 'format' driver in qemu */ - TEST_JSON_FORMAT_NET("<source protocol='http' name=''>\n" - " <host name='example.com' port='80'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='http' name='file'>\n" - " <host name='example.com' port='80'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='https' name='file'>\n" - " <host name='example.com' port='432'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='gluster' name='vol/file'>\n" - " <host name='example.com' port='24007'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='gluster' name='testvol/img.qcow2'>\n" - " <host name='example.com' port='1234'/>\n" - " <host transport='unix' socket='/path/socket'/>\n" - " <host name='example.com' port='24007'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='nbd'>\n" - " <host transport='unix' socket='/path/to/socket'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='nbd' name='blah'>\n" - " <host name='example.org' port='6000'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='nbd'>\n" - " <host name='example.org' port='6000'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='ssh' name='blah'>\n" - " <host name='example.org' port='6000'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='rbd' name='libvirt/test'>\n" - " <host name='example.com' port='1234'/>\n" - " <host name='example2.com'/>\n" - " <snapshot name='snapshotname'/>\n" - " <config file='/path/to/conf'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/0'>\n" - " <host name='test.org' port='3260'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/6'>\n" - " <host name='test.org' port='1234'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='sheepdog' name='test'>\n" - " <host name='example.com' port='321'/>\n" - "</source>\n"); - TEST_JSON_FORMAT_NET("<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" - " <host name='example.com' port='9999'/>\n" - "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='http' name=''>\n" + " <host name='example.com' port='80'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='http' name='file'>\n" + " <host name='example.com' port='80'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='https' name='file'>\n" + " <host name='example.com' port='432'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='gluster' name='vol/file'>\n" + " <host name='example.com' port='24007'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='gluster' name='testvol/img.qcow2'>\n" + " <host name='example.com' port='1234'/>\n" + " <host transport='unix' socket='/path/socket'/>\n" + " <host name='example.com' port='24007'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='nbd'>\n" + " <host transport='unix' socket='/path/to/socket'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='nbd' name='blah'>\n" + " <host name='example.org' port='6000'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='nbd'>\n" + " <host name='example.org' port='6000'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='ssh' name='blah'>\n" + " <host name='example.org' port='6000'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='rbd' name='libvirt/test'>\n" + " <host name='example.com' port='1234'/>\n" + " <host name='example2.com'/>\n" + " <snapshot name='snapshotname'/>\n" + " <config file='/path/to/conf'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/0'>\n" + " <host name='test.org' port='3260'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/6'>\n" + " <host name='test.org' port='1234'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='sheepdog' name='test'>\n" + " <host name='example.com' port='321'/>\n" + "</source>\n"); + TEST_JSON_FORMAT("<source type='network' format='raw' protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='example.com' port='9999'/>\n" + "</source>\n"); # define TEST_DISK_TO_JSON_FULL(nme, fl) \ do { \ -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:12PM +0100, Peter Krempa wrote:
This is part of the effort to minimize use of virDomainDiskSourceParse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 130 ++++++++++++++++++++---------------------- 1 file changed, 61 insertions(+), 69 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use the new parser instead of virDomainDiskSourceParse. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 38 +++++++++++--------------------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index bc4b9c8f11..5dad2c3c51 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -108,16 +108,10 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, { int ret = -1; char *snapshot = NULL; - char *type = NULL; - char *driver = NULL; - xmlNodePtr cur; xmlNodePtr saved = ctxt->node; ctxt->node = node; - if (!(def->src = virStorageSourceNew())) - goto cleanup; - def->name = virXMLPropString(node, "name"); if (!def->name) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -136,27 +130,19 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, } } - if ((type = virXMLPropString(node, "type"))) { - if ((def->src->type = virStorageTypeFromString(type)) <= 0 || - def->src->type == VIR_STORAGE_TYPE_VOLUME || - def->src->type == VIR_STORAGE_TYPE_DIR) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown disk snapshot type '%s'"), type); - goto cleanup; - } - } else { - def->src->type = VIR_STORAGE_TYPE_FILE; - } - - if ((cur = virXPathNode("./source", ctxt)) && - virDomainDiskSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) + if (!(def->src = virDomainStorageSourceParseFull("string(@type)", + "string(./driver/@type)", + "./source", + NULL, true, + ctxt, flags, xmlopt))) goto cleanup; - if ((driver = virXPathString("string(./driver/@type)", ctxt)) && - (def->src->format = virStorageFileFormatTypeFromString(driver)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk snapshot driver '%s'"), driver); - goto cleanup; + if (def->src->type == VIR_STORAGE_TYPE_VOLUME || + def->src->type == VIR_STORAGE_TYPE_DIR) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported disk snapshot type '%s'"), + virStorageTypeToString(def->src->type)); + goto cleanup; } /* validate that the passed path is absolute */ @@ -174,9 +160,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, cleanup: ctxt->node = saved; - VIR_FREE(driver); VIR_FREE(snapshot); - VIR_FREE(type); if (ret < 0) virDomainSnapshotDiskDefClear(def); return ret; -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:13PM +0100, Peter Krempa wrote:
Use the new parser instead of virDomainDiskSourceParse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 38 +++++++++++--------------------------- 1 file changed, 11 insertions(+), 27 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virDomainDiskSourceParse was now just a thin wrapper without any extra value. Replace all usage of it by the function it calls and remove the function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 22 ++++------------------ src/conf/domain_conf.h | 5 ----- 2 files changed, 4 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e7f3bcf114..3de72fd807 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9205,20 +9205,6 @@ virDomainStorageSourceParseFull(const char *typeXPath, } -int -virDomainDiskSourceParse(xmlNodePtr node, - xmlXPathContextPtr ctxt, - virStorageSourcePtr src, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - if (virDomainStorageSourceParse(node, ctxt, src, flags, xmlopt) < 0) - return -1; - - return 0; -} - - static int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virStorageSourcePtr src, @@ -9280,7 +9266,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, return -1; } - if (virDomainDiskSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || + if (virDomainStorageSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) return -1; @@ -9414,8 +9400,8 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, return -1; } - if (virDomainDiskSourceParse(mirrorNode, ctxt, def->mirror, - flags, xmlopt) < 0) + if (virDomainStorageSourceParse(mirrorNode, ctxt, def->mirror, + flags, xmlopt) < 0) return -1; } else { /* For back-compat reasons, we handle a file name @@ -9865,7 +9851,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, continue; if (!source && virXMLNodeNameEqual(cur, "source")) { - if (virDomainDiskSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) + if (virDomainStorageSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) goto error; /* If we've already found an <auth> as a child of <disk> and diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ad288d702b..72272965ae 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3070,11 +3070,6 @@ virDomainDiskDefPtr virDomainDiskRemove(virDomainDefPtr def, size_t i); virDomainDiskDefPtr virDomainDiskRemoveByName(virDomainDefPtr def, const char *name); -int virDomainDiskSourceParse(xmlNodePtr node, - xmlXPathContextPtr ctxt, - virStorageSourcePtr src, - unsigned int flags, - virDomainXMLOptionPtr xmlopt); int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net); virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char *device); -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:14PM +0100, Peter Krempa wrote:
virDomainDiskSourceParse was now just a thin wrapper without any extra value. Replace all usage of it by the function it calls and remove the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 22 ++++------------------ src/conf/domain_conf.h | 5 ----- 2 files changed, 4 insertions(+), 23 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Replace most of the function by using the existing parsing helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 53 +++++++++--------------------------------- 1 file changed, 11 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3de72fd807..d77d3befd3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9212,62 +9212,31 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virDomainXMLOptionPtr xmlopt) { VIR_XPATH_NODE_AUTORESTORE(ctxt); - xmlNodePtr source; VIR_AUTOUNREF(virStorageSourcePtr) backingStore = NULL; VIR_AUTOFREE(char *) type = NULL; - VIR_AUTOFREE(char *) format = NULL; - VIR_AUTOFREE(char *) idx = NULL; if (!(ctxt->node = virXPathNode("./backingStore", ctxt))) return 0; - if (!(backingStore = virStorageSourceNew())) - return -1; - - /* backing store is always read-only */ - backingStore->readonly = true; - /* terminator does not have a type */ if (!(type = virXMLPropString(ctxt->node, "type"))) { - VIR_STEAL_PTR(src->backingStore, backingStore); - return 0; - } - - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && - (idx = virXMLPropString(ctxt->node, "index")) && - virStrToLong_uip(idx, NULL, 10, &backingStore->id) < 0) { - virReportError(VIR_ERR_XML_ERROR, _("invalid disk index '%s'"), idx); - return -1; - } - - backingStore->type = virStorageTypeFromString(type); - if (backingStore->type <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk backing store type '%s'"), type); - return -1; - } + if (!(src->backingStore = virStorageSourceNew())) + return -1; - if (!(format = virXPathString("string(./format/@type)", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing disk backing store format")); - return -1; + return 0; } - backingStore->format = virStorageFileFormatTypeFromString(format); - if (backingStore->format <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk backing store format '%s'"), format); + if (!(backingStore = virDomainStorageSourceParseFull("string(@type)", + "string(./format/@type)", + "./source", + "string(@index)", + false, ctxt, flags, xmlopt))) return -1; - } - if (!(source = virXPathNode("./source", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing disk backing store source")); - return -1; - } + /* backing store is always read-only */ + backingStore->readonly = true; - if (virDomainStorageSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || - virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) + if (virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) return -1; VIR_STEAL_PTR(src->backingStore, backingStore); -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:15PM +0100, Peter Krempa wrote:
Replace most of the function by using the existing parsing helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 53 +++++++++--------------------------------- 1 file changed, 11 insertions(+), 42 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use the new parser function for disk source. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d77d3befd3..ec666d8d8c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9783,17 +9783,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = node; /* 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, @@ -9815,14 +9806,18 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, rawio = virXMLPropString(node, "rawio"); sgio = virXMLPropString(node, "sgio"); + virObjectUnref(def->src); + if (!(def->src = virDomainStorageSourceParseFull("string(@type)", NULL, + "./source", + "string(./source/@index)", + true, ctxt, flags, xmlopt))) + goto error; + for (cur = node->children; cur != NULL; cur = cur->next) { if (cur->type != XML_ELEMENT_NODE) continue; if (!source && virXMLNodeNameEqual(cur, "source")) { - if (virDomainStorageSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) - goto error; - /* If we've already found an <auth> as a child of <disk> and * we find one as a child of <source>, then force an error to * avoid ambiguity */ @@ -9851,14 +9846,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, source = true; startupPolicy = virXMLPropString(cur, "startupPolicy"); - - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && - (tmp = virXMLPropString(cur, "index")) && - virStrToLong_uip(tmp, NULL, 10, &def->src->id) < 0) { - virReportError(VIR_ERR_XML_ERROR, _("invalid disk index '%s'"), tmp); - goto error; - } - VIR_FREE(tmp); } else if (!target && virXMLNodeNameEqual(cur, "target")) { target = virXMLPropString(cur, "dev"); -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:16PM +0100, Peter Krempa wrote:
Use the new parser function for disk source.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ec666d8d8c..5790b19315 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9334,15 +9334,11 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, unsigned int flags, virDomainXMLOptionPtr xmlopt) { - xmlNodePtr mirrorNode; VIR_AUTOFREE(char *) mirrorFormat = NULL; VIR_AUTOFREE(char *) mirrorType = NULL; VIR_AUTOFREE(char *) ready = NULL; VIR_AUTOFREE(char *) blockJob = NULL; - if (!(def->mirror = virStorageSourceNew())) - return -1; - if ((blockJob = virXMLPropString(cur, "job"))) { if ((def->mirrorJob = virDomainBlockJobTypeFromString(blockJob)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -9354,25 +9350,18 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, } if ((mirrorType = virXMLPropString(cur, "type"))) { - if ((def->mirror->type = virStorageTypeFromString(mirrorType)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown mirror backing store type '%s'"), - mirrorType); + if (!(def->mirror = virDomainStorageSourceParseFull("string(./mirror/@type)", + NULL, + "./mirror/source", + NULL, + false, ctxt, flags, xmlopt))) return -1; - } mirrorFormat = virXPathString("string(./mirror/format/@type)", ctxt); - - if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("mirror requires source element")); + } else { + if (!(def->mirror = virStorageSourceNew())) return -1; - } - if (virDomainStorageSourceParse(mirrorNode, ctxt, def->mirror, - flags, xmlopt) < 0) - return -1; - } else { /* For back-compat reasons, we handle a file name * encoded as attributes, even though we prefer * modern output in the style of backingStore */ -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:17PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move parsing of <backingStore> into virDomainStorageSourceParseFull so that it can be reused easily. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 100 +++++++++++++++++++++------------------ src/conf/domain_conf.h | 3 +- src/conf/snapshot_conf.c | 2 +- src/qemu/qemu_domain.c | 2 +- tests/qemublocktest.c | 2 +- 5 files changed, 59 insertions(+), 50 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5790b19315..5b13402154 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9111,6 +9111,43 @@ virDomainStorageSourceParse(xmlNodePtr node, } +static int +virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, + virStorageSourcePtr src, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) +{ + VIR_XPATH_NODE_AUTORESTORE(ctxt); + VIR_AUTOUNREF(virStorageSourcePtr) backingStore = NULL; + VIR_AUTOFREE(char *) type = NULL; + + if (!(ctxt->node = virXPathNode("./backingStore", ctxt))) + return 0; + + /* terminator does not have a type */ + if (!(type = virXMLPropString(ctxt->node, "type"))) { + if (!(src->backingStore = virStorageSourceNew())) + return -1; + + return 0; + } + + if (!(backingStore = virDomainStorageSourceParseFull("string(@type)", + "string(./format/@type)", + "./source", + "string(@index)", + false, true, ctxt, flags, xmlopt))) + return -1; + + /* backing store is always read-only */ + backingStore->readonly = true; + + VIR_STEAL_PTR(src->backingStore, backingStore); + + return 0; +} + + /** * virDomainStorageSourceParseFull * @typeXPath: XPath query string for the 'type' of virStorageSource @@ -9118,6 +9155,7 @@ virDomainStorageSourceParse(xmlNodePtr node, * @sourceXPath: XPath query for the <source> subelement * @indexXPath: XPath query for 'id' in virStorageSource (may be NULL if skipped) * @allowMissing: if true no errors are reported if the above fields are missing + * @backingStore: Full backing chain is parsed if true * @ctxt: XPath context * @flags: XML parser flags * @xmlopt: XML parser callbacks @@ -9128,6 +9166,10 @@ virDomainStorageSourceParse(xmlNodePtr node, * are missing. @formatXPath and @indexXpath may be NULL if they should be omitted * or if the caller parses the value separately. * + * @backingStore controls whether the full backing chain should be parsed. + * Backing chain is parsed from elements named <backingStore> relative to + * the node of @ctxt when called. + * * Returns the parsed source or NULL on error. */ virStorageSourcePtr @@ -9136,6 +9178,7 @@ virDomainStorageSourceParseFull(const char *typeXPath, const char *sourceXPath, const char *indexXPath, bool allowMissing, + bool backingStore, xmlXPathContextPtr ctxt, unsigned int flags, virDomainXMLOptionPtr xmlopt) @@ -9200,50 +9243,15 @@ virDomainStorageSourceParseFull(const char *typeXPath, virDomainStorageSourceParse(sourceNode, ctxt, src, flags, xmlopt) < 0) return NULL; + if (backingStore && + virDomainDiskBackingStoreParse(ctxt, src, flags, xmlopt) < 0) + return NULL; + VIR_STEAL_PTR(ret, src); return ret; } -static int -virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, - virStorageSourcePtr src, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - VIR_XPATH_NODE_AUTORESTORE(ctxt); - VIR_AUTOUNREF(virStorageSourcePtr) backingStore = NULL; - VIR_AUTOFREE(char *) type = NULL; - - if (!(ctxt->node = virXPathNode("./backingStore", ctxt))) - return 0; - - /* terminator does not have a type */ - if (!(type = virXMLPropString(ctxt->node, "type"))) { - if (!(src->backingStore = virStorageSourceNew())) - return -1; - - return 0; - } - - if (!(backingStore = virDomainStorageSourceParseFull("string(@type)", - "string(./format/@type)", - "./source", - "string(@index)", - false, ctxt, flags, xmlopt))) - return -1; - - /* backing store is always read-only */ - backingStore->readonly = true; - - if (virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) - return -1; - - VIR_STEAL_PTR(src->backingStore, backingStore); - - return 0; -} - #define PARSE_IOTUNE(val) \ if (virXPathULongLong("string(./iotune/" #val ")", \ ctxt, &def->blkdeviotune.val) == -2) { \ @@ -9354,7 +9362,7 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, NULL, "./mirror/source", NULL, - false, ctxt, flags, xmlopt))) + false, false, ctxt, flags, xmlopt))) return -1; mirrorFormat = virXPathString("string(./mirror/format/@type)", ctxt); @@ -9765,6 +9773,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_AUTOFREE(char *) vendor = NULL; VIR_AUTOFREE(char *) product = NULL; VIR_AUTOFREE(char *) domain_name = NULL; + bool backingStore = true; if (!(def = virDomainDiskDefNew(xmlopt))) return NULL; @@ -9774,6 +9783,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, /* defaults */ def->device = VIR_DOMAIN_DISK_DEVICE_DISK; + if ((flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) + backingStore = false; + if ((tmp = virXMLPropString(node, "device")) && (def->device = virDomainDiskDeviceTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -9799,7 +9811,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (!(def->src = virDomainStorageSourceParseFull("string(@type)", NULL, "./source", "string(./source/@index)", - true, ctxt, flags, xmlopt))) + true, backingStore, + ctxt, flags, xmlopt))) goto error; for (cur = node->children; cur != NULL; cur = cur->next) { @@ -10134,11 +10147,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_STEAL_PTR(def->vendor, vendor); VIR_STEAL_PTR(def->product, product); - if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { - if (virDomainDiskBackingStoreParse(ctxt, def->src, flags, xmlopt) < 0) - goto error; - } - if (flags & VIR_DOMAIN_DEF_PARSE_STATUS && virDomainDiskDefParsePrivateData(ctxt, def, xmlopt) < 0) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 72272965ae..7ab7b28e4f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3470,11 +3470,12 @@ virDomainStorageSourceParseFull(const char *typeXPath, const char *sourceXPath, const char *indexXPath, bool allowMissing, + bool backingStore, xmlXPathContextPtr ctxt, unsigned int flags, virDomainXMLOptionPtr xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) - ATTRIBUTE_NONNULL(6); + ATTRIBUTE_NONNULL(7); int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, int maplen, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 5dad2c3c51..4cc4795cf4 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -133,7 +133,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, if (!(def->src = virDomainStorageSourceParseFull("string(@type)", "string(./driver/@type)", "./source", - NULL, true, + NULL, true, false, ctxt, flags, xmlopt))) goto cleanup; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e406647d8f..3ae9bdad1a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2696,7 +2696,7 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, if (!(diskPriv->migrSource = virDomainStorageSourceParseFull("string(@type)", "string(@format)", ".", NULL, - false, ctxt, + false, false, ctxt, VIR_DOMAIN_DEF_PARSE_STATUS, priv->driver->xmlopt))) return -1; diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 48cec2869b..09eaf155e9 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -57,7 +57,7 @@ testBackingXMLjsonXML(const void *args) if (!(xmlsrc = virDomainStorageSourceParseFull("string(./@type)", "string(./@format)", - ".", NULL, false, + ".", NULL, false, false, ctxt, 0, NULL))) { fprintf(stderr, "failed to parse disk source xml\n"); return -1; -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:18PM +0100, Peter Krempa wrote:
Move parsing of <backingStore> into virDomainStorageSourceParseFull so that it can be reused easily.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 100 +++++++++++++++++++++------------------ src/conf/domain_conf.h | 3 +- src/conf/snapshot_conf.c | 2 +- src/qemu/qemu_domain.c | 2 +- tests/qemublocktest.c | 2 +- 5 files changed, 59 insertions(+), 50 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5790b19315..5b13402154 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9111,6 +9111,43 @@ virDomainStorageSourceParse(xmlNodePtr node, }
+static int +virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, + virStorageSourcePtr src, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) +{ + VIR_XPATH_NODE_AUTORESTORE(ctxt); + VIR_AUTOUNREF(virStorageSourcePtr) backingStore = NULL; + VIR_AUTOFREE(char *) type = NULL; + + if (!(ctxt->node = virXPathNode("./backingStore", ctxt))) + return 0; + + /* terminator does not have a type */ + if (!(type = virXMLPropString(ctxt->node, "type"))) { + if (!(src->backingStore = virStorageSourceNew())) + return -1; + + return 0; + } + + if (!(backingStore = virDomainStorageSourceParseFull("string(@type)", + "string(./format/@type)", + "./source", + "string(@index)", + false, true, ctxt, flags, xmlopt))) + return -1; + + /* backing store is always read-only */ + backingStore->readonly = true; + + VIR_STEAL_PTR(src->backingStore, backingStore); + + return 0; +} + +
The movement of this function should be in a separate commit. This one would only remove the virDomainDiskBackingStoreParse call and add the 'true' argument.
/** * virDomainStorageSourceParseFull * @typeXPath: XPath query string for the 'type' of virStorageSource @@ -9774,6 +9783,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, /* defaults */ def->device = VIR_DOMAIN_DISK_DEVICE_DISK;
+ if ((flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE))
Too many parentheses.
+ backingStore = false; + if ((tmp = virXMLPropString(node, "device")) && (def->device = virDomainDiskDeviceTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When the block copy operation is started with a reused external file in incremental mode libvirt will need to open and insert the backing chain for that file into qemu (in -blockdev mode). This means that we'll need to track the backing chain and metadata such as node names for the full chain of <mirror>. This patch invokes the full backing chain formatter and parser for <mirror> so that the chain can be kept with <mirror>. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 13 ++++++++----- tests/qemustatusxml2xmldata/blockjob-mirror-in.xml | 13 +++++++++++++ tests/qemuxml2argvdata/disk-mirror.xml | 6 ++++++ tests/qemuxml2xmloutdata/disk-mirror-active.xml | 6 ++++++ 5 files changed, 34 insertions(+), 5 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1828e0795b..623ef28719 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5676,6 +5676,7 @@ </choice> </attribute> </optional> + <ref name="diskBackingChain"/> </element> </define> <define name="diskAuth"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5b13402154..585b5515f9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9342,6 +9342,7 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, unsigned int flags, virDomainXMLOptionPtr xmlopt) { + VIR_XPATH_NODE_AUTORESTORE(ctxt); VIR_AUTOFREE(char *) mirrorFormat = NULL; VIR_AUTOFREE(char *) mirrorType = NULL; VIR_AUTOFREE(char *) ready = NULL; @@ -9358,14 +9359,16 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, } if ((mirrorType = virXMLPropString(cur, "type"))) { - if (!(def->mirror = virDomainStorageSourceParseFull("string(./mirror/@type)", + ctxt->node = cur; + + if (!(def->mirror = virDomainStorageSourceParseFull("string(./@type)", NULL, - "./mirror/source", + "./source", NULL, - false, false, ctxt, flags, xmlopt))) + false, true, ctxt, flags, xmlopt))) return -1; - mirrorFormat = virXPathString("string(./mirror/format/@type)", ctxt); + mirrorFormat = virXPathString("string(./format/@type)", ctxt); } else { if (!(def->mirror = virStorageSourceNew())) return -1; @@ -24127,7 +24130,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(buf, disk->mirror, 0, 0, true, false, false, + if (virDomainDiskSourceFormat(buf, disk->mirror, 0, flags, true, false, true, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/tests/qemustatusxml2xmldata/blockjob-mirror-in.xml b/tests/qemustatusxml2xmldata/blockjob-mirror-in.xml index 32bde1ba66..df23ac00aa 100644 --- a/tests/qemustatusxml2xmldata/blockjob-mirror-in.xml +++ b/tests/qemustatusxml2xmldata/blockjob-mirror-in.xml @@ -65,6 +65,7 @@ <mirror type='file' file='/tmp/copy.img' format='qcow2' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> + <backingStore/> </mirror> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> @@ -76,6 +77,18 @@ <mirror type='file' file='/tmp/logcopy.img' format='qcow2' job='copy' ready='abort'> <format type='qcow2'/> <source file='/tmp/logcopy.img'/> + <backingStore type='block' index='1'> + <format type='raw'/> + <source dev='/dev/HostVG/backing'> + <privateData> + <nodenames> + <nodename type='storage' name='test-backing-storage'/> + <nodename type='format' name='test-backing-format'/> + </nodenames> + </privateData> + </source> + <backingStore/> + </backingStore> </mirror> <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> diff --git a/tests/qemuxml2argvdata/disk-mirror.xml b/tests/qemuxml2argvdata/disk-mirror.xml index e89eee47ed..c1e6e94e33 100644 --- a/tests/qemuxml2argvdata/disk-mirror.xml +++ b/tests/qemuxml2argvdata/disk-mirror.xml @@ -36,6 +36,7 @@ <mirror type='file' file='/tmp/copy.img' format='qcow2' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> + <backingStore/> </mirror> <target dev='vda' bus='virtio'/> </disk> @@ -45,6 +46,11 @@ <mirror type='file' file='/tmp/logcopy.img' format='qcow2' job='copy' ready='abort'> <format type='qcow2'/> <source file='/tmp/logcopy.img'/> + <backingStore type='block' index='1'> + <format type='raw'/> + <source dev='/dev/HostVG/backing'/> + <backingStore/> + </backingStore> </mirror> <target dev='vdb' bus='virtio'/> </disk> diff --git a/tests/qemuxml2xmloutdata/disk-mirror-active.xml b/tests/qemuxml2xmloutdata/disk-mirror-active.xml index d689eac6b8..32ffc647be 100644 --- a/tests/qemuxml2xmloutdata/disk-mirror-active.xml +++ b/tests/qemuxml2xmloutdata/disk-mirror-active.xml @@ -40,6 +40,7 @@ <mirror type='file' file='/tmp/copy.img' format='qcow2' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> + <backingStore/> </mirror> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> @@ -51,6 +52,11 @@ <mirror type='file' file='/tmp/logcopy.img' format='qcow2' job='copy' ready='abort'> <format type='qcow2'/> <source file='/tmp/logcopy.img'/> + <backingStore type='block' index='1'> + <format type='raw'/> + <source dev='/dev/HostVG/backing'/> + <backingStore/> + </backingStore> </mirror> <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:19PM +0100, Peter Krempa wrote:
When the block copy operation is started with a reused external file in incremental mode libvirt will need to open and insert the backing chain for that file into qemu (in -blockdev mode). This means that we'll need to track the backing chain and metadata such as node names for the full chain of <mirror>.
This patch invokes the full backing chain formatter and parser for <mirror> so that the chain can be kept with <mirror>.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 13 ++++++++----- tests/qemustatusxml2xmldata/blockjob-mirror-in.xml | 13 +++++++++++++ tests/qemuxml2argvdata/disk-mirror.xml | 6 ++++++ tests/qemuxml2xmloutdata/disk-mirror-active.xml | 6 ++++++ 5 files changed, 34 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Similarly to the disk source we need to keep the disk index (which is in the qemu driver used for identification of the source for block jobs) for the <mirror> element so that when it's replaced as a disk source after pivoting all the allocated data is present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 4 ++-- tests/qemuxml2argvdata/disk-mirror.xml | 4 ++-- tests/qemuxml2xmloutdata/disk-mirror-active.xml | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 585b5515f9..2bb4d6eafc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9364,7 +9364,7 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, if (!(def->mirror = virDomainStorageSourceParseFull("string(./@type)", NULL, "./source", - NULL, + "string(./source/@index)", false, true, ctxt, flags, xmlopt))) return -1; @@ -24130,7 +24130,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(buf, disk->mirror, 0, flags, true, false, true, + if (virDomainDiskSourceFormat(buf, disk->mirror, 0, flags, true, true, true, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/tests/qemuxml2argvdata/disk-mirror.xml b/tests/qemuxml2argvdata/disk-mirror.xml index c1e6e94e33..5a825c54ac 100644 --- a/tests/qemuxml2argvdata/disk-mirror.xml +++ b/tests/qemuxml2argvdata/disk-mirror.xml @@ -45,8 +45,8 @@ <backingStore/> <mirror type='file' file='/tmp/logcopy.img' format='qcow2' job='copy' ready='abort'> <format type='qcow2'/> - <source file='/tmp/logcopy.img'/> - <backingStore type='block' index='1'> + <source file='/tmp/logcopy.img' index='1'/> + <backingStore type='block' index='2'> <format type='raw'/> <source dev='/dev/HostVG/backing'/> <backingStore/> diff --git a/tests/qemuxml2xmloutdata/disk-mirror-active.xml b/tests/qemuxml2xmloutdata/disk-mirror-active.xml index 32ffc647be..bebdb849c2 100644 --- a/tests/qemuxml2xmloutdata/disk-mirror-active.xml +++ b/tests/qemuxml2xmloutdata/disk-mirror-active.xml @@ -51,8 +51,8 @@ <backingStore/> <mirror type='file' file='/tmp/logcopy.img' format='qcow2' job='copy' ready='abort'> <format type='qcow2'/> - <source file='/tmp/logcopy.img'/> - <backingStore type='block' index='1'> + <source file='/tmp/logcopy.img' index='1'/> + <backingStore type='block' index='2'> <format type='raw'/> <source dev='/dev/HostVG/backing'/> <backingStore/> -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:20PM +0100, Peter Krempa wrote:
Similarly to the disk source we need to keep the disk index (which is in the qemu driver used for identification of the source for block jobs) for the <mirror> element so that when it's replaced as a disk source after pivoting all the allocated data is present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 4 ++-- tests/qemuxml2argvdata/disk-mirror.xml | 4 ++-- tests/qemuxml2xmloutdata/disk-mirror-active.xml | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We parse the seclabels and use them internally so omitting them when formatting would be misleading. Additionally our schema actually allows them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 3 +-- tests/qemuxml2argvdata/disk-backing-chains.xml | 6 +++++- tests/qemuxml2xmloutdata/disk-backing-chains-active.xml | 6 +++++- tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml | 6 +++++- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2bb4d6eafc..105ece2b7c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23892,8 +23892,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAsprintf(&childBuf, "<format type='%s'/>\n", virStorageFileFormatTypeToString(backingStore->format)); - /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, false, + if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, true, false, true, xmlopt) < 0) return -1; diff --git a/tests/qemuxml2argvdata/disk-backing-chains.xml b/tests/qemuxml2argvdata/disk-backing-chains.xml index 703c60c439..2eaafb34e7 100644 --- a/tests/qemuxml2argvdata/disk-backing-chains.xml +++ b/tests/qemuxml2argvdata/disk-backing-chains.xml @@ -38,7 +38,11 @@ <source file='/tmp/image2.qcow'/> <backingStore type='file' index='3'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file' index='4'> <format type='qcow2'/> <source file='/tmp/image4.qcow'/> diff --git a/tests/qemuxml2xmloutdata/disk-backing-chains-active.xml b/tests/qemuxml2xmloutdata/disk-backing-chains-active.xml index d1fd2442c3..e24956f106 100644 --- a/tests/qemuxml2xmloutdata/disk-backing-chains-active.xml +++ b/tests/qemuxml2xmloutdata/disk-backing-chains-active.xml @@ -39,7 +39,11 @@ <source file='/tmp/image2.qcow'/> <backingStore type='file' index='3'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file' index='4'> <format type='qcow2'/> <source file='/tmp/image4.qcow'/> diff --git a/tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml b/tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml index c1af58ff6f..e39e218873 100644 --- a/tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml +++ b/tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml @@ -39,7 +39,11 @@ <source file='/tmp/image2.qcow'/> <backingStore type='file'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file'> <format type='qcow2'/> <source file='/tmp/image4.qcow'/> -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:21PM +0100, Peter Krempa wrote:
We parse the seclabels and use them internally so omitting them when formatting would be misleading. Additionally our schema actually allows them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 3 +-- tests/qemuxml2argvdata/disk-backing-chains.xml | 6 +++++- tests/qemuxml2xmloutdata/disk-backing-chains-active.xml | 6 +++++- tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml | 6 +++++- 4 files changed, 16 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
All callers pass in 'true' so we can remove the parameter. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 9 ++++----- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 2 +- tests/virstoragetest.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 105ece2b7c..d12a8dcaf9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23892,7 +23892,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAsprintf(&childBuf, "<format type='%s'/>\n", virStorageFileFormatTypeToString(backingStore->format)); - if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, true, + if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, flags, false, true, xmlopt) < 0) return -1; @@ -23953,7 +23953,6 @@ virDomainDiskSourceFormat(virBufferPtr buf, virStorageSourcePtr src, int policy, unsigned int flags, - bool seclabels, bool attrIndex, bool backingStore, virDomainXMLOptionPtr xmlopt) @@ -23964,7 +23963,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, virBufferSetChildIndent(&childBuf, buf); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - seclabels, attrIndex, + true, attrIndex, policy, xmlopt) < 0) return -1; @@ -24129,7 +24128,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(buf, disk->mirror, 0, flags, true, true, true, + if (virDomainDiskSourceFormat(buf, disk->mirror, 0, flags, true, true, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); @@ -24227,7 +24226,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virStorageAuthDefFormat(buf, def->src->auth); if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy, - flags, true, true, true, xmlopt) < 0) + flags, true, true, xmlopt) < 0) return -1; virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7ab7b28e4f..26c6f45685 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3016,7 +3016,6 @@ int virDomainDiskSourceFormat(virBufferPtr buf, virStorageSourcePtr src, int policy, unsigned int flags, - bool seclabels, bool attrIndex, bool backingStore, virDomainXMLOptionPtr xmlopt); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 4cc4795cf4..da27dcc1a1 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -752,7 +752,7 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (disk->src->format > 0) virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->src->format)); - if (virDomainDiskSourceFormat(buf, disk->src, 0, 0, true, false, false, xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, disk->src, 0, 0, false, false, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 9a11f5bfe8..ba97a80d45 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -657,7 +657,7 @@ testBackingParse(const void *args) goto cleanup; } - if (virDomainDiskSourceFormat(&buf, src, 0, 0, true, false, false, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, src, 0, 0, false, false, NULL) < 0 || !(xml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); goto cleanup; -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:22PM +0100, Peter Krempa wrote:
All callers pass in 'true' so we can remove the parameter.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 9 ++++----- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 2 +- tests/virstoragetest.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
All callers now pass true so we can remove the argument. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d12a8dcaf9..51bacfdd82 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23758,7 +23758,6 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, * @childBuf: buffer for subelements of the formatted element * @src: storage source to format * @flags: XML formatter flags - * @seclabels: security labels are formatted if true * @attrIndex: the 'index' attribute is formatted if true * @policy: startup policy, taken from disk (use 0 to omit) * @xmlopt: XML options data (for private data formatters) @@ -23774,7 +23773,6 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, unsigned int flags, - bool seclabels, bool attrIndex, int policy, virDomainXMLOptionPtr xmlopt) @@ -23817,7 +23815,7 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, return -1; } - if (seclabels && src->type != VIR_STORAGE_TYPE_NETWORK) + if (src->type != VIR_STORAGE_TYPE_NETWORK) virDomainSourceDefFormatSeclabel(childBuf, src->nseclabels, src->seclabels, flags); @@ -23934,7 +23932,7 @@ virDomainStorageSourceFormatFull(virBufferPtr buf, virStorageFileFormatTypeToString(src->format)); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - true, true, 0, xmlopt) < 0) + true, 0, xmlopt) < 0) return -1; if (virXMLFormatElement(buf, elemname, &attrBuf, &childBuf) < 0) @@ -23963,8 +23961,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, virBufferSetChildIndent(&childBuf, buf); if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - true, attrIndex, - policy, xmlopt) < 0) + attrIndex, policy, xmlopt) < 0) return -1; if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) -- 2.20.1
On Mon, Mar 18, 2019 at 04:55:23PM +0100, Peter Krempa wrote:
All callers now pass true so we can remove the argument.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa