[libvirt] [PATCH v2 00/29] conf: refactor virStorageSource parsing and formatting (blockdev-add saga)
Most patches were changed too substantially to warrant keeping the Rb's. Peter Krempa (29): conf: Format seclabels for <backingStore> conf: Remove @seclabels from virDomainStorageSourceFormat conf: Merge virDomainDiskSourceFormatInternal into virDomainDiskSourceFormat conf: Export virDomainDiskSourceFormat tests: qemuxml2xml: Use virdeterministichashmock.so tests: qemustatusxml2xml: Add another disk to migration-out-nbd-tls case tests: qemustatusxml2xml: Add separate output for migration-out-nbd-tls qemu: domain: Modify <migrationSource> to look like <disk> conf: Unexport virDomainStorageSourceFormat conf: Simplify control flow in virDomainDiskSourceFormat conf: Avoid temporary variable in virDomainDiskBackingStoreFormat conf: Use virXMLFormatElement in virDomainDiskBackingStoreFormat conf: Merge virDomainStorageSourceFormat into virDomainDiskSourceFormat conf: Document virDomainDiskSourceFormat conf: Replace virDomainDiskSourceParse by virDomainStorageSourceParse conf: introduce virDomainStorageSourceParseBase conf: Use virDomainStorageSourceParseBase in virDomainDiskBackingStoreParse conf: Document virDomainStorageSourceParse conf: Modify arguments passed to virDomainDiskBackingStoreFormat conf: Allow convenient lookup of <source> in virDomainStorageSourceParse qemu: Use VIR_AUTOFREE in qemuDomainObjPrivateXMLParseJobNBDSource qemu: Remove cleanup in qemuDomainObjPrivateXMLParseJobNBDSource qemu: Use virDomainStorageSourceParseBase in qemuDomainObjPrivateXMLParseJobNBDSource qemu: Parse NBD storage source private data by virDomainStorageSourceParse conf: use virXMLFormatElement in virDomainDiskDefFormatMirror conf: Pass 'flags' to virDomainDiskSourceFormat in virDomainDiskDefFormatMirror conf: Refactor virDomainDiskDefMirrorParse conf: Parse and format 'backingStore' for disk <mirror> conf: Add 'index' attribute for <disk><mirror><source> docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 345 ++++++------- src/conf/domain_conf.h | 19 +- src/conf/snapshot_conf.c | 4 +- src/libvirt_private.syms | 3 +- src/qemu/qemu_domain.c | 82 +-- tests/qemublocktest.c | 4 +- .../blockjob-mirror-in.xml | 13 + .../migration-out-nbd-tls-in.xml | 27 +- .../migration-out-nbd-tls-out.xml | 488 +++++++++++++++++- .../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/qemuxml2xmltest.c | 3 +- tests/virstoragetest.c | 2 +- 17 files changed, 769 insertions(+), 256 deletions(-) mode change 120000 => 100644 tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml -- 2.20.1
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 9d8eabb42a..87db233d9f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23876,8 +23876,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<format type='%s'/>\n", format); - /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, false, + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, true, false, xmlopt) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, xmlopt, flags) < 0) 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 Fri, Mar 22, 2019 at 07:00:37PM +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 including transitive callers through virDomainDiskSourceFormatInternal always pass true. Remove the argument. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 15 ++++++--------- src/conf/domain_conf.h | 3 +-- src/qemu/qemu_domain.c | 2 +- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 87db233d9f..69fd4a7c57 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23720,8 +23720,7 @@ int virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, - unsigned int flags, - bool seclabels) + unsigned int flags) { switch ((virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: @@ -23761,7 +23760,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); @@ -23793,7 +23792,6 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virStorageSourcePtr src, int policy, unsigned int flags, - bool seclabels, bool attrIndex, virDomainXMLOptionPtr xmlopt) { @@ -23803,8 +23801,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virBufferSetChildIndent(&childBuf, buf); - if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, - seclabels) < 0) + if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags) < 0) goto cleanup; if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) @@ -23834,7 +23831,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, unsigned int flags, virDomainXMLOptionPtr xmlopt) { - return virDomainDiskSourceFormatInternal(buf, src, policy, flags, true, + return virDomainDiskSourceFormatInternal(buf, src, policy, flags, false, xmlopt); } @@ -23876,7 +23873,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<format type='%s'/>\n", format); - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, true, + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, false, xmlopt) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, xmlopt, flags) < 0) @@ -24135,7 +24132,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virStorageAuthDefFormat(buf, def->src->auth); if (virDomainDiskSourceFormatInternal(buf, def->src, def->startupPolicy, - flags, true, true, xmlopt) < 0) + flags, 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 4a25480662..25c10a9af3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3459,8 +3459,7 @@ int virDomainDiskDefCheckDuplicateInfo(const virDomainDiskDef *a, int virDomainStorageSourceFormat(virBufferPtr attrBuf, virBufferPtr childBuf, virStorageSourcePtr src, - unsigned int flags, - bool seclabels) + unsigned int flags) 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 9ca4ca33e5..a1b39220d1 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, true) < 0) + VIR_DOMAIN_DEF_FORMAT_STATUS) < 0) goto cleanup; if (qemuStorageSourcePrivateDataFormat(src, &privateDataBuf) < 0) -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:38PM +0100, Peter Krempa wrote:
All callers including transitive callers through virDomainDiskSourceFormatInternal always pass true. Remove the argument.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 15 ++++++--------- src/conf/domain_conf.h | 3 +-- src/qemu/qemu_domain.c | 2 +- 3 files changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove the wrapper and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 35 +++++++++++------------------------ src/conf/domain_conf.h | 1 + src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- tests/virstoragetest.c | 2 +- 5 files changed, 15 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 69fd4a7c57..adb20e33f2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23787,13 +23787,13 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, } -static int -virDomainDiskSourceFormatInternal(virBufferPtr buf, - virStorageSourcePtr src, - int policy, - unsigned int flags, - bool attrIndex, - virDomainXMLOptionPtr xmlopt) +int +virDomainDiskSourceFormat(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + bool attrIndex, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; @@ -23824,18 +23824,6 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, } -int -virDomainDiskSourceFormat(virBufferPtr buf, - virStorageSourcePtr src, - int policy, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - return virDomainDiskSourceFormatInternal(buf, src, policy, flags, - false, xmlopt); -} - - static int virDomainDiskBackingStoreFormat(virBufferPtr buf, virStorageSourcePtr backingStore, @@ -23873,8 +23861,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<format type='%s'/>\n", format); - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, flags, - false, xmlopt) < 0 || + if (virDomainDiskSourceFormat(buf, backingStore, 0, false, flags, xmlopt) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, xmlopt, flags) < 0) return -1; @@ -24035,7 +24022,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, false, 0, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</mirror>\n"); @@ -24131,8 +24118,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, xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy, true, + flags, 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 25c10a9af3..f0c2a51212 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3014,6 +3014,7 @@ int virDomainDefFormatInternal(virDomainDefPtr def, int virDomainDiskSourceFormat(virBufferPtr buf, virStorageSourcePtr src, int policy, + bool attrIndex, unsigned int flags, virDomainXMLOptionPtr xmlopt); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 52abafab0f..58d2cfee99 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -776,7 +776,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, false, 0, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index becd31a0ad..46e75c2df5 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -86,7 +86,7 @@ testBackingXMLjsonXML(const void *args) return -1; } - if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, false, 0, NULL) < 0 || !(actualxml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); return -1; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index fb98903f02..d29181dea9 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, false, 0, NULL) < 0 || !(xml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); goto cleanup; -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:39PM +0100, Peter Krempa wrote:
Remove the wrapper and fix callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 35 +++++++++++------------------------ src/conf/domain_conf.h | 1 + src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- tests/virstoragetest.c | 2 +- 5 files changed, 15 insertions(+), 27 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bf5625fbf4..c26c525d37 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -354,6 +354,7 @@ virDomainDiskSetDriver; virDomainDiskSetFormat; virDomainDiskSetSource; virDomainDiskSetType; +virDomainDiskSourceFormat; virDomainDiskTranslateSourcePool; virDomainFeatureTypeFromString; virDomainFeatureTypeToString; -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:40PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Block job related data will be stored in a has table and formatted into the status XML. Use the mock to guarantee stable tests. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2xmltest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 83a0d1cf7b..408d20a92a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1305,7 +1305,8 @@ mymain(void) VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virpcimock.so", - abs_builddir "/.libs/virrandommock.so") + abs_builddir "/.libs/virrandommock.so", + abs_builddir "/.libs/virdeterministichashmock.so") #else -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:41PM +0100, Peter Krempa wrote:
Block job related data will be stored in a has table and formatted into the status XML. Use the mock to guarantee stable tests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2xmltest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Upcomming change will modify some aspects. To allow testing upgrade path add another disk. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../migration-out-nbd-tls-in.xml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml index be1dc7347e..f3bbc752b6 100644 --- a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml +++ b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml @@ -247,6 +247,17 @@ </privateData> </migrationSource> </disk> + <disk dev='vdc' migrating='yes'> + <migrationSource type='network' format='raw' protocol='nbd' name='drive-virtio-disk2' tlsFromConfig='0'> + <host name='andariel.usersys.redhat.com' port='49152'/> + <privateData> + <nodenames> + <nodename type='storage' name='migration-vdc-storage'/> + <nodename type='format' name='migration-vdc-format'/> + </nodenames> + </privateData> + </migrationSource> + </disk> <disk dev='hda' migrating='no'/> </job> <devices> @@ -334,6 +345,14 @@ <alias name='virtio-disk1'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/b.qcow2'/> + <backingStore/> + <target dev='vdc' bus='virtio'/> + <alias name='virtio-disk2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> + </disk> <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso'/> -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:42PM +0100, Peter Krempa wrote:
Upcomming change will modify some aspects. To allow testing upgrade path add another disk.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../migration-out-nbd-tls-in.xml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Upcomming change will modify some aspects. To allow testing upgrade path add a separate output file so that we can see the conversion from old to new config. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../migration-out-nbd-tls-out.xml | 484 +++++++++++++++++- 1 file changed, 483 insertions(+), 1 deletion(-) mode change 120000 => 100644 tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml deleted file mode 120000 index e8cdbdb55e..0000000000 --- a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml +++ /dev/null @@ -1 +0,0 @@ -migration-out-nbd-tls-in.xml \ No newline at end of file diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml new file mode 100644 index 0000000000..f3bbc752b6 --- /dev/null +++ b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml @@ -0,0 +1,483 @@ +<domstatus state='running' reason='booted' pid='68472'> + <taint flag='high-privileges'/> + <monitor path='/var/lib/libvirt/qemu/domain-3-upstream/monitor.sock' json='1' type='unix'/> + <namespaces> + <mount/> + </namespaces> + <vcpus> + <vcpu id='0' pid='68477'/> + <vcpu id='1' pid='68478'/> + </vcpus> + <qemuCaps> + <flag name='kvm'/> + <flag name='mem-path'/> + <flag name='drive-serial'/> + <flag name='monitor-json'/> + <flag name='sdl'/> + <flag name='netdev'/> + <flag name='rtc'/> + <flag name='vhost-net'/> + <flag name='no-hpet'/> + <flag name='no-kvm-pit'/> + <flag name='nodefconfig'/> + <flag name='boot-menu'/> + <flag name='fsdev'/> + <flag name='name-process'/> + <flag name='smbios-type'/> + <flag name='spice'/> + <flag name='vga-none'/> + <flag name='boot-index'/> + <flag name='hda-duplex'/> + <flag name='drive-aio'/> + <flag name='ccid-emulated'/> + <flag name='ccid-passthru'/> + <flag name='chardev-spicevmc'/> + <flag name='virtio-tx-alg'/> + <flag name='pci-multifunction'/> + <flag name='virtio-blk-pci.ioeventfd'/> + <flag name='sga'/> + <flag name='virtio-blk-pci.event_idx'/> + <flag name='virtio-net-pci.event_idx'/> + <flag name='cache-directsync'/> + <flag name='piix3-usb-uhci'/> + <flag name='piix4-usb-uhci'/> + <flag name='usb-ehci'/> + <flag name='ich9-usb-ehci1'/> + <flag name='vt82c686b-usb-uhci'/> + <flag name='pci-ohci'/> + <flag name='usb-redir'/> + <flag name='usb-hub'/> + <flag name='no-shutdown'/> + <flag name='cache-unsafe'/> + <flag name='ich9-ahci'/> + <flag name='no-acpi'/> + <flag name='fsdev-readonly'/> + <flag name='virtio-blk-pci.scsi'/> + <flag name='drive-copy-on-read'/> + <flag name='fsdev-writeout'/> + <flag name='drive-iotune'/> + <flag name='system_wakeup'/> + <flag name='scsi-disk.channel'/> + <flag name='scsi-block'/> + <flag name='transaction'/> + <flag name='block-job-async'/> + <flag name='scsi-cd'/> + <flag name='ide-cd'/> + <flag name='no-user-config'/> + <flag name='hda-micro'/> + <flag name='dump-guest-memory'/> + <flag name='nec-usb-xhci'/> + <flag name='balloon-event'/> + <flag name='bridge'/> + <flag name='lsi'/> + <flag name='virtio-scsi-pci'/> + <flag name='blockio'/> + <flag name='disable-s3'/> + <flag name='disable-s4'/> + <flag name='usb-redir.filter'/> + <flag name='ide-drive.wwn'/> + <flag name='scsi-disk.wwn'/> + <flag name='seccomp-sandbox'/> + <flag name='reboot-timeout'/> + <flag name='dump-guest-core'/> + <flag name='seamless-migration'/> + <flag name='block-commit'/> + <flag name='vnc'/> + <flag name='drive-mirror'/> + <flag name='usb-redir.bootindex'/> + <flag name='usb-host.bootindex'/> + <flag name='blockdev-snapshot-sync'/> + <flag name='qxl'/> + <flag name='VGA'/> + <flag name='cirrus-vga'/> + <flag name='vmware-svga'/> + <flag name='device-video-primary'/> + <flag name='usb-serial'/> + <flag name='usb-net'/> + <flag name='add-fd'/> + <flag name='nbd-server'/> + <flag name='virtio-rng'/> + <flag name='rng-random'/> + <flag name='rng-egd'/> + <flag name='dtb'/> + <flag name='megasas'/> + <flag name='ipv6-migration'/> + <flag name='machine-opt'/> + <flag name='machine-usb-opt'/> + <flag name='tpm-passthrough'/> + <flag name='tpm-tis'/> + <flag name='pci-bridge'/> + <flag name='vfio-pci'/> + <flag name='vfio-pci.bootindex'/> + <flag name='scsi-generic'/> + <flag name='scsi-generic.bootindex'/> + <flag name='mem-merge'/> + <flag name='vnc-websocket'/> + <flag name='drive-discard'/> + <flag name='mlock'/> + <flag name='vnc-share-policy'/> + <flag name='device-del-event'/> + <flag name='dmi-to-pci-bridge'/> + <flag name='i440fx-pci-hole64-size'/> + <flag name='q35-pci-hole64-size'/> + <flag name='usb-storage'/> + <flag name='usb-storage.removable'/> + <flag name='virtio-mmio'/> + <flag name='ich9-intel-hda'/> + <flag name='kvm-pit-lost-tick-policy'/> + <flag name='boot-strict'/> + <flag name='pvpanic'/> + <flag name='spice-file-xfer-disable'/> + <flag name='spiceport'/> + <flag name='usb-kbd'/> + <flag name='host-pci-multidomain'/> + <flag name='msg-timestamp'/> + <flag name='active-commit'/> + <flag name='change-backing-file'/> + <flag name='memory-backend-ram'/> + <flag name='numa'/> + <flag name='memory-backend-file'/> + <flag name='usb-audio'/> + <flag name='rtc-reset-reinjection'/> + <flag name='splash-timeout'/> + <flag name='iothread'/> + <flag name='migrate-rdma'/> + <flag name='ivshmem'/> + <flag name='drive-iotune-max'/> + <flag name='VGA.vgamem_mb'/> + <flag name='vmware-svga.vgamem_mb'/> + <flag name='qxl.vgamem_mb'/> + <flag name='pc-dimm'/> + <flag name='machine-vmport-opt'/> + <flag name='aes-key-wrap'/> + <flag name='dea-key-wrap'/> + <flag name='pci-serial'/> + <flag name='vhost-user-multiqueue'/> + <flag name='migration-event'/> + <flag name='ioh3420'/> + <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> + <flag name='rtl8139'/> + <flag name='e1000'/> + <flag name='virtio-net'/> + <flag name='gic-version'/> + <flag name='incoming-defer'/> + <flag name='virtio-gpu'/> + <flag name='virtio-gpu.virgl'/> + <flag name='virtio-keyboard'/> + <flag name='virtio-mouse'/> + <flag name='virtio-tablet'/> + <flag name='virtio-input-host'/> + <flag name='chardev-file-append'/> + <flag name='ich9-disable-s3'/> + <flag name='ich9-disable-s4'/> + <flag name='vserport-change-event'/> + <flag name='virtio-balloon-pci.deflate-on-oom'/> + <flag name='mptsas1068'/> + <flag name='qxl.vram64_size_mb'/> + <flag name='chardev-logfile'/> + <flag name='debug-threads'/> + <flag name='secret'/> + <flag name='pxb'/> + <flag name='pxb-pcie'/> + <flag name='device-tray-moved-event'/> + <flag name='nec-usb-xhci-ports'/> + <flag name='virtio-scsi-pci.iothread'/> + <flag name='name-guest'/> + <flag name='qxl.max_outputs'/> + <flag name='spice-unix'/> + <flag name='drive-detect-zeroes'/> + <flag name='tls-creds-x509'/> + <flag name='display'/> + <flag name='intel-iommu'/> + <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> + <flag name='query-hotpluggable-cpus'/> + <flag name='virtio-net.rx_queue_size'/> + <flag name='virtio-vga'/> + <flag name='drive-iotune-max-length'/> + <flag name='ivshmem-plain'/> + <flag name='ivshmem-doorbell'/> + <flag name='query-qmp-schema'/> + <flag name='gluster.debug_level'/> + <flag name='vhost-scsi'/> + <flag name='drive-iotune-group'/> + <flag name='query-cpu-model-expansion'/> + <flag name='virtio-net.host_mtu'/> + <flag name='nvdimm'/> + <flag name='pcie-root-port'/> + <flag name='query-cpu-definitions'/> + <flag name='block-write-threshold'/> + <flag name='query-named-block-nodes'/> + <flag name='cpu-cache'/> + <flag name='qemu-xhci'/> + <flag name='kernel-irqchip'/> + <flag name='kernel-irqchip.split'/> + <flag name='intel-iommu.intremap'/> + <flag name='intel-iommu.caching-mode'/> + <flag name='intel-iommu.eim'/> + <flag name='intel-iommu.device-iotlb'/> + <flag name='virtio.iommu_platform'/> + <flag name='virtio.ats'/> + <flag name='loadparm'/> + <flag name='vnc-multi-servers'/> + <flag name='virtio-net.tx_queue_size'/> + <flag name='chardev-reconnect'/> + <flag name='virtio-gpu.max_outputs'/> + <flag name='vxhs'/> + <flag name='virtio-blk.num-queues'/> + <flag name='vmcoreinfo'/> + <flag name='numa.dist'/> + <flag name='disk-share-rw'/> + <flag name='iscsi.password-secret'/> + <flag name='isa-serial'/> + <flag name='dump-completed'/> + <flag name='nbd-tls'/> + <flag name='blockdev-del'/> + </qemuCaps> + <job type='none' async='migration out' phase='perform3' flags='0x0'> + <disk dev='vdb' migrating='yes'> + <migrationSource type='network' format='raw' protocol='nbd' name='drive-virtio-disk1' tlsFromConfig='0'> + <host name='andariel.usersys.redhat.com' port='49152'/> + <privateData> + <nodenames> + <nodename type='storage' name='migration-vdb-storage'/> + <nodename type='format' name='migration-vdb-format'/> + </nodenames> + </privateData> + </migrationSource> + </disk> + <disk dev='vdc' migrating='yes'> + <migrationSource type='network' format='raw' protocol='nbd' name='drive-virtio-disk2' tlsFromConfig='0'> + <host name='andariel.usersys.redhat.com' port='49152'/> + <privateData> + <nodenames> + <nodename type='storage' name='migration-vdc-storage'/> + <nodename type='format' name='migration-vdc-format'/> + </nodenames> + </privateData> + </migrationSource> + </disk> + <disk dev='hda' migrating='no'/> + </job> + <devices> + <device alias='rng0'/> + <device alias='sound0-codec0'/> + <device alias='virtio-disk1'/> + <device alias='virtio-serial0'/> + <device alias='video0'/> + <device alias='serial0'/> + <device alias='sound0'/> + <device alias='balloon0'/> + <device alias='channel1'/> + <device alias='channel0'/> + <device alias='net0'/> + <device alias='input0'/> + <device alias='redir0'/> + <device alias='redir1'/> + <device alias='scsi0'/> + <device alias='usb'/> + <device alias='ide0-0-0'/> + </devices> + <numad nodeset='0' cpuset='0-7'/> + <libDir path='/var/lib/libvirt/qemu/domain-3-upstream'/> + <channelTargetDir path='/var/lib/libvirt/qemu/channel/target/domain-3-upstream'/> + <chardevStdioLogd/> + <allowReboot value='yes'/> + <blockjobs active='no'/> + <domain type='kvm' id='3'> + <name>upstream</name> + <uuid>dcf47dbd-46d1-4d5b-b442-262a806a333a</uuid> + <memory unit='KiB'>1024000</memory> + <currentMemory unit='KiB'>1024000</currentMemory> + <memoryBacking> + <access mode='shared'/> + </memoryBacking> + <vcpu placement='auto' current='2'>8</vcpu> + <numatune> + <memory mode='strict' placement='auto'/> + </numatune> + <resource> + <partition>/machine</partition> + </resource> + <os> + <type arch='x86_64' machine='pc-i440fx-2.9'>hvm</type> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <vmport state='off'/> + </features> + <cpu> + <numa> + <cell id='0' cpus='0,2,4,6' memory='512000' unit='KiB'/> + <cell id='1' cpus='1,3,5,7' memory='512000' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'> + <timer name='rtc' tickpolicy='catchup'/> + <timer name='pit' tickpolicy='delay'/> + <timer name='hpet' present='no'/> + </clock> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='no'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' discard='unmap' detect_zeroes='on'/> + <source file='/var/lib/libvirt/images/a.qcow2'/> + <backingStore type='file' index='1'> + <format type='qcow2'/> + <source file='/var/lib/libvirt/images/base.qcow2'> + <privateData> + <relPath>base.qcow2</relPath> + </privateData> + </source> + <backingStore/> + </backingStore> + <target dev='vdb' bus='virtio'/> + <alias name='virtio-disk1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/b.qcow2'/> + <backingStore/> + <target dev='vdc' bus='virtio'/> + <alias name='virtio-disk2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso'/> + <backingStore/> + <target dev='hda' bus='ide'/> + <readonly/> + <boot order='1'/> + <alias name='ide0-0-0'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0' model='ich9-ehci1'> + <alias name='usb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <alias name='usb'/> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <alias name='usb'/> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <alias name='usb'/> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci.0'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </controller> + <controller type='ide' index='0'> + <alias name='ide'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='lsilogic'> + <alias name='scsi0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + </controller> + <controller type='fdc' index='0'> + <alias name='fdc0'/> + </controller> + <interface type='network'> + <mac address='52:54:00:36:bd:3b'/> + <source network='default'/> + <actual type='network'> + <source bridge='virbr0'/> + </actual> + <target dev='vnet0'/> + <model type='virtio'/> + <alias name='net0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <serial type='pty'> + <source path='/dev/pts/67'/> + <target type='isa-serial' port='0'> + <model name='isa-serial'/> + </target> + <alias name='serial0'/> + </serial> + <console type='pty' tty='/dev/pts/67'> + <source path='/dev/pts/67'/> + <target type='serial' port='0'/> + <alias name='serial0'/> + </console> + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-3-upstream/org.qemu.guest_agent.0'/> + <target type='virtio' name='org.qemu.guest_agent.0' state='disconnected'/> + <alias name='channel0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <channel type='spicevmc'> + <target type='virtio' name='com.redhat.spice.0' state='disconnected'/> + <alias name='channel1'/> + <address type='virtio-serial' controller='0' bus='0' port='2'/> + </channel> + <input type='tablet' bus='usb'> + <alias name='input0'/> + <address type='usb' bus='0' port='1'/> + </input> + <input type='mouse' bus='ps2'> + <alias name='input1'/> + </input> + <input type='keyboard' bus='ps2'> + <alias name='input2'/> + </input> + <graphics type='spice' port='5900' autoport='yes' listen='127.0.0.1'> + <listen type='address' address='127.0.0.1' fromConfig='1' autoGenerated='no'/> + <image compression='off'/> + </graphics> + <sound model='ich6'> + <alias name='sound0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </sound> + <video> + <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1' primary='yes'/> + <alias name='video0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <redirdev bus='usb' type='spicevmc'> + <alias name='redir0'/> + <address type='usb' bus='0' port='2'/> + </redirdev> + <redirdev bus='usb' type='spicevmc'> + <alias name='redir1'/> + <address type='usb' bus='0' port='3'/> + </redirdev> + <memballoon model='virtio'> + <alias name='balloon0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </memballoon> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <alias name='rng0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </rng> + </devices> + <seclabel type='dynamic' model='dac' relabel='yes'> + <label>+0:+0</label> + <imagelabel>+0:+0</imagelabel> + </seclabel> + </domain> +</domstatus> -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:43PM +0100, Peter Krempa wrote:
Upcomming change will modify some aspects. To allow testing upgrade path add a separate output file so that we can see the conversion from old to new config.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../migration-out-nbd-tls-out.xml | 484 +++++++++++++++++- 1 file changed, 483 insertions(+), 1 deletion(-) mode change 120000 => 100644 tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When adding <migrationSource> I've used a slightly unusual approach. To allow using the disk source XML parser and formatter convert <migrationSource> to look like <disk>. This means that <source> will be added as a subelement of <migrationSource> rather than being formatted inline. Conversion from the old format in the parser is very simple as it involves only moving the XPath context current node slightly if the new format is found. The status XML to XML test shows that the upgrade is done correctly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++------ .../migration-out-nbd-tls-in.xml | 18 +++++----- .../migration-out-nbd-tls-out.xml | 36 ++++++++++--------- 3 files changed, 41 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a1b39220d1..3b0887a194 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2343,28 +2343,21 @@ qemuDomainObjPrivateXMLFormatPR(virBufferPtr buf, static int qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, - virStorageSourcePtr src) + virStorageSourcePtr src, + virDomainXMLOptionPtr xmlopt) { 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) < 0) - goto cleanup; - - if (qemuStorageSourcePrivateDataFormat(src, &privateDataBuf) < 0) - goto cleanup; - - if (virXMLFormatElement(&childBuf, "privateData", NULL, &privateDataBuf) < 0) + if (virDomainDiskSourceFormat(&childBuf, src, 0, false, + VIR_DOMAIN_DEF_FORMAT_STATUS, xmlopt) < 0) goto cleanup; if (virXMLFormatElement(buf, "migrationSource", &attrBuf, &childBuf) < 0) @@ -2381,6 +2374,7 @@ 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,7 +2393,8 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, if (diskPriv->migrSource && qemuDomainObjPrivateXMLFormatNBDMigrationSource(&childBuf, - diskPriv->migrSource) < 0) + diskPriv->migrSource, + priv->driver->xmlopt) < 0) goto cleanup; if (virXMLFormatElement(buf, "disk", &attrBuf, &childBuf) < 0) @@ -2724,6 +2719,7 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, char *type = NULL; int ret = -1; VIR_AUTOUNREF(virStorageSourcePtr) migrSource = NULL; + xmlNodePtr sourceNode; ctxt->node = node; @@ -2759,6 +2755,9 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, goto cleanup; } + if ((sourceNode = virXPathNode("./source", ctxt))) + ctxt->node = sourceNode; + if (virDomainStorageSourceParse(ctxt->node, ctxt, migrSource, VIR_DOMAIN_DEF_PARSE_STATUS, NULL) < 0) goto cleanup; diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml index f3bbc752b6..2cd6c9a5e9 100644 --- a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml +++ b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-in.xml @@ -237,14 +237,16 @@ </qemuCaps> <job type='none' async='migration out' phase='perform3' flags='0x0'> <disk dev='vdb' migrating='yes'> - <migrationSource type='network' format='raw' protocol='nbd' name='drive-virtio-disk1' tlsFromConfig='0'> - <host name='andariel.usersys.redhat.com' port='49152'/> - <privateData> - <nodenames> - <nodename type='storage' name='migration-vdb-storage'/> - <nodename type='format' name='migration-vdb-format'/> - </nodenames> - </privateData> + <migrationSource type='network' format='raw'> + <source protocol='nbd' name='drive-virtio-disk1' tlsFromConfig='0'> + <host name='andariel.usersys.redhat.com' port='49152'/> + <privateData> + <nodenames> + <nodename type='storage' name='migration-vdb-storage'/> + <nodename type='format' name='migration-vdb-format'/> + </nodenames> + </privateData> + </source> </migrationSource> </disk> <disk dev='vdc' migrating='yes'> diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml index f3bbc752b6..869f37d488 100644 --- a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml +++ b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml @@ -237,25 +237,29 @@ </qemuCaps> <job type='none' async='migration out' phase='perform3' flags='0x0'> <disk dev='vdb' migrating='yes'> - <migrationSource type='network' format='raw' protocol='nbd' name='drive-virtio-disk1' tlsFromConfig='0'> - <host name='andariel.usersys.redhat.com' port='49152'/> - <privateData> - <nodenames> - <nodename type='storage' name='migration-vdb-storage'/> - <nodename type='format' name='migration-vdb-format'/> - </nodenames> - </privateData> + <migrationSource type='network' format='raw'> + <source protocol='nbd' name='drive-virtio-disk1' tlsFromConfig='0'> + <host name='andariel.usersys.redhat.com' port='49152'/> + <privateData> + <nodenames> + <nodename type='storage' name='migration-vdb-storage'/> + <nodename type='format' name='migration-vdb-format'/> + </nodenames> + </privateData> + </source> </migrationSource> </disk> <disk dev='vdc' migrating='yes'> - <migrationSource type='network' format='raw' protocol='nbd' name='drive-virtio-disk2' tlsFromConfig='0'> - <host name='andariel.usersys.redhat.com' port='49152'/> - <privateData> - <nodenames> - <nodename type='storage' name='migration-vdc-storage'/> - <nodename type='format' name='migration-vdc-format'/> - </nodenames> - </privateData> + <migrationSource type='network' format='raw'> + <source protocol='nbd' name='drive-virtio-disk2' tlsFromConfig='0'> + <host name='andariel.usersys.redhat.com' port='49152'/> + <privateData> + <nodenames> + <nodename type='storage' name='migration-vdc-storage'/> + <nodename type='format' name='migration-vdc-format'/> + </nodenames> + </privateData> + </source> </migrationSource> </disk> <disk dev='hda' migrating='no'/> -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:44PM +0100, Peter Krempa wrote:
When adding <migrationSource> I've used a slightly unusual approach. To allow using the disk source XML parser and formatter convert <migrationSource> to look like <disk>. This means that <source> will be added as a subelement of <migrationSource> rather than being formatted inline.
Conversion from the old format in the parser is very simple as it involves only moving the XPath context current node slightly if the new format is found.
The status XML to XML test shows that the upgrade is done correctly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++------ .../migration-out-nbd-tls-in.xml | 18 +++++----- .../migration-out-nbd-tls-out.xml | 36 ++++++++++--------- 3 files changed, 41 insertions(+), 36 deletions(-)
@@ -2759,6 +2755,9 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, goto cleanup; }
This deserves a comment. One of these should do: /* newer libvirt uses the <source> subelement instead of formatting the * source directly into <migrationSource> */ /* crazy backcompat stuff */ /* prefer the <source> subelement if present */ /* this is source */
+ if ((sourceNode = virXPathNode("./source", ctxt))) + ctxt->node = sourceNode; + if (virDomainStorageSourceParse(ctxt->node, ctxt, migrSource, VIR_DOMAIN_DEF_PARSE_STATUS, NULL) < 0) goto cleanup;
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 | 5 ----- src/libvirt_private.syms | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index adb20e33f2..ec81d380d5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23716,7 +23716,7 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, } -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 f0c2a51212..cca81be013 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3457,11 +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) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c26c525d37..681ff911b6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -564,7 +564,6 @@ virDomainStateReasonToString; virDomainStateTypeFromString; virDomainStateTypeToString; virDomainStorageNetworkParseHost; -virDomainStorageSourceFormat; virDomainStorageSourceParse; virDomainTaintTypeFromString; virDomainTaintTypeToString; -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:45PM +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 | 5 ----- src/libvirt_private.syms | 1 - 3 files changed, 1 insertion(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Now that the cleanup is handled automatically it can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ec81d380d5..fa4454a6ca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23797,12 +23797,11 @@ 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) < 0) - goto cleanup; + return -1; if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) virBufferEscapeString(&attrBuf, " startupPolicy='%s'", @@ -23812,15 +23811,12 @@ virDomainDiskSourceFormat(virBufferPtr buf, virBufferAsprintf(&attrBuf, " index='%u'", src->id); if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) - goto cleanup; + return -1; if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:46PM +0100, Peter Krempa wrote:
Now that the cleanup is handled automatically it can be removed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 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 fa4454a6ca..5adca7c29c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23826,7 +23826,6 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virDomainXMLOptionPtr xmlopt, unsigned int flags) { - const char *format; bool inactive = flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE; if (!backingStore) @@ -23841,8 +23840,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); @@ -23856,7 +23854,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)); if (virDomainDiskSourceFormat(buf, backingStore, 0, false, flags, xmlopt) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, xmlopt, flags) < 0) -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:47PM +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 | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5adca7c29c..89e2900df2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23826,8 +23826,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; @@ -23847,22 +23851,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)); - if (virDomainDiskSourceFormat(buf, backingStore, 0, false, flags, xmlopt) < 0 || - virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, + if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, false, flags, xmlopt) < 0) + return -1; + + if (virDomainDiskBackingStoreFormat(&childBuf, backingStore->backingStore, xmlopt, flags) < 0) return -1; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</backingStore>\n"); + if (virXMLFormatElement(buf, "backingStore", &attrBuf, &childBuf) < 0) + return -1; + return 0; } -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:48PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There was only one caller, remove the unnecessary wrapper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89e2900df2..db02005f5b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23716,38 +23716,45 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, } -static int -virDomainStorageSourceFormat(virBufferPtr attrBuf, - virBufferPtr childBuf, - virStorageSourcePtr src, - unsigned int flags) +int +virDomainDiskSourceFormat(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + bool attrIndex, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; + + virBufferSetChildIndent(&childBuf, buf); + switch ((virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: - virBufferEscapeString(attrBuf, " file='%s'", src->path); + virBufferEscapeString(&attrBuf, " file='%s'", src->path); break; case VIR_STORAGE_TYPE_BLOCK: - virBufferEscapeString(attrBuf, " dev='%s'", src->path); + virBufferEscapeString(&attrBuf, " dev='%s'", src->path); break; case VIR_STORAGE_TYPE_DIR: - virBufferEscapeString(attrBuf, " dir='%s'", src->path); + virBufferEscapeString(&attrBuf, " dir='%s'", src->path); break; case VIR_STORAGE_TYPE_NETWORK: - if (virDomainDiskSourceFormatNetwork(attrBuf, childBuf, + if (virDomainDiskSourceFormatNetwork(&attrBuf, &childBuf, src, flags) < 0) return -1; break; case VIR_STORAGE_TYPE_VOLUME: if (src->srcpool) { - virBufferEscapeString(attrBuf, " pool='%s'", src->srcpool->pool); - virBufferEscapeString(attrBuf, " volume='%s'", + virBufferEscapeString(&attrBuf, " pool='%s'", src->srcpool->pool); + virBufferEscapeString(&attrBuf, " volume='%s'", src->srcpool->volume); if (src->srcpool->mode) - virBufferAsprintf(attrBuf, " mode='%s'", + virBufferAsprintf(&attrBuf, " mode='%s'", virStorageSourcePoolModeTypeToString(src->srcpool->mode)); } @@ -23761,7 +23768,7 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, } if (src->type != VIR_STORAGE_TYPE_NETWORK) - virDomainSourceDefFormatSeclabel(childBuf, src->nseclabels, + virDomainSourceDefFormatSeclabel(&childBuf, src->nseclabels, src->seclabels, flags); /* Storage Source formatting will not carry through the blunder @@ -23771,38 +23778,17 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, * So avoid formatting it for volumes. */ if (src->auth && src->authInherited && src->type != VIR_STORAGE_TYPE_VOLUME) - virStorageAuthDefFormat(childBuf, src->auth); + virStorageAuthDefFormat(&childBuf, src->auth); /* If we found encryption as a child of <source>, then format it * as we found it. */ if (src->encryption && src->encryptionInherited && - virStorageEncryptionFormat(childBuf, src->encryption) < 0) + virStorageEncryptionFormat(&childBuf, src->encryption) < 0) return -1; if (src->pr) - virStoragePRDefFormat(childBuf, src->pr, + virStoragePRDefFormat(&childBuf, src->pr, flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE); - - return 0; -} - - -int -virDomainDiskSourceFormat(virBufferPtr buf, - virStorageSourcePtr src, - int policy, - bool attrIndex, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; - VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; - - virBufferSetChildIndent(&childBuf, buf); - - if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags) < 0) - return -1; - if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) virBufferEscapeString(&attrBuf, " startupPolicy='%s'", virDomainStartupPolicyTypeToString(policy)); -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:49PM +0100, Peter Krempa wrote:
There was only one caller, remove the unnecessary wrapper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 37 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 3/22/19 1:00 PM, Peter Krempa wrote:
There was only one caller, remove the unnecessary wrapper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 37 deletions(-)
Alas, my backup code wants to be a second caller. https://www.redhat.com/archives/libvir-list/2019-March/msg00393.html virDomainStorageSourceFormat was a nice independent function that did not care what element it was being formatted to; virDomainDiskSourceFormat always formats into <disk ....>. But backup wants to format into <target> (push mode) or <scratch> (pull mode). So I'll be including a revert of this patch in my next round of incremental backup patches. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 4/4/19 3:16 PM, Eric Blake wrote:
On 3/22/19 1:00 PM, Peter Krempa wrote:
There was only one caller, remove the unnecessary wrapper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 37 deletions(-)
Alas, my backup code wants to be a second caller. https://www.redhat.com/archives/libvir-list/2019-March/msg00393.html
virDomainStorageSourceFormat was a nice independent function that did not care what element it was being formatted to; virDomainDiskSourceFormat always formats into <disk ....>.
But backup wants to format into <target> (push mode) or <scratch> (pull mode). So I'll be including a revert of this patch in my next round of incremental backup patches.
Or maybe just tweaking it to add a parameter that says what string name to use for the overall element. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Apr 04, 2019 at 15:29:30 -0500, Eric Blake wrote:
On 4/4/19 3:16 PM, Eric Blake wrote:
On 3/22/19 1:00 PM, Peter Krempa wrote:
There was only one caller, remove the unnecessary wrapper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 37 deletions(-)
Alas, my backup code wants to be a second caller. https://www.redhat.com/archives/libvir-list/2019-March/msg00393.html
virDomainStorageSourceFormat was a nice independent function that did not care what element it was being formatted to; virDomainDiskSourceFormat always formats into <disk ....>.
But backup wants to format into <target> (push mode) or <scratch> (pull mode). So I'll be including a revert of this patch in my next round of incremental backup patches.
Or maybe just tweaking it to add a parameter that says what string name to use for the overall element.
I prefer this one. If you need the element to be something else than 'source' please add a parameter. Passing back a buffer was a somewhat failed experiment which allowed to merge 'type' and 'format' attributes into the <source> (or equivalent) element. This design enforces users to add a wrapper element similarly to what other XMLs are having. Original idea was to simplify thing but the parser didn't turn out simpler at all and all the old XML design can't be changed anyways.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index db02005f5b..fa50b6f000 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23716,6 +23716,18 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, } +/** + * virDomainDiskSourceFormat: + * @buf: output buffer + * @src: storage source definition to format + * @policy: startup policy attribute value, if necessary + * @attrIndex: the 'index' attribute of <source> is formatted if true + * @flags: XML formatter flags + * @xmlopt: XML formatter callbacks + * + * Formats @src into a <source> element. Note that this doesn't format the + * 'type' and 'format' properties of @src. + */ int virDomainDiskSourceFormat(virBufferPtr buf, virStorageSourcePtr src, -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:50PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
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 ----- src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- 4 files changed, 6 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fa50b6f000..c93c4df929 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9111,20 +9111,6 @@ virDomainStorageSourceParse(xmlNodePtr node, } -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, @@ -9186,7 +9172,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; @@ -9320,8 +9306,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 @@ -9771,7 +9757,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 cca81be013..f550c257d6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3067,11 +3067,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); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 58d2cfee99..93b5bafcd6 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -146,7 +146,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, } if ((cur = virXPathNode("./source", ctxt)) && - virDomainDiskSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) + virDomainStorageSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) goto cleanup; if ((driver = virXPathString("string(./driver/@type)", ctxt)) && diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 46e75c2df5..1a365a411c 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -62,7 +62,7 @@ testBackingXMLjsonXML(const void *args) if (!(xml = virXMLParseStringCtxt(data->xml, "(test storage source XML)", &ctxt))) return -1; - if (virDomainDiskSourceParse(ctxt->node, ctxt, xmlsrc, 0, NULL) < 0) { + if (virDomainStorageSourceParse(ctxt->node, ctxt, xmlsrc, 0, NULL) < 0) { fprintf(stderr, "failed to parse disk source xml\n"); return -1; } -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:51PM +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 ----- src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- 4 files changed, 6 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fa50b6f000..c93c4df929 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9186,7 +9172,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, return -1; }
- if (virDomainDiskSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || + if (virDomainStorageSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 ||
Hmm, so we have virDomainStorageSourceParse and a corresponding virDomainDiskSourceFormat? Maybe the previous commit dropped the wrong function.
virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) return -1;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Tue, Apr 02, 2019 at 09:35:36 +0200, Ján Tomko wrote:
On Fri, Mar 22, 2019 at 07:00:51PM +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 ----- src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- 4 files changed, 6 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fa50b6f000..c93c4df929 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9186,7 +9172,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, return -1; }
- if (virDomainDiskSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || + if (virDomainStorageSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 ||
Hmm, so we have virDomainStorageSourceParse and a corresponding virDomainDiskSourceFormat?
Maybe the previous commit dropped the wrong function.
Should I rename virDomainDiskSourceFormat to virDomainStorageSourceFormat in an upcomming patch?
virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) return -1;
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Apr 02, 2019 at 02:09:24PM +0200, Peter Krempa wrote:
On Tue, Apr 02, 2019 at 09:35:36 +0200, Ján Tomko wrote:
On Fri, Mar 22, 2019 at 07:00:51PM +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 ----- src/conf/snapshot_conf.c | 2 +- tests/qemublocktest.c | 2 +- 4 files changed, 6 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fa50b6f000..c93c4df929 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9186,7 +9172,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, return -1; }
- if (virDomainDiskSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || + if (virDomainStorageSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 ||
Hmm, so we have virDomainStorageSourceParse and a corresponding virDomainDiskSourceFormat?
Maybe the previous commit dropped the wrong function.
Should I rename virDomainDiskSourceFormat to virDomainStorageSourceFormat in an upcomming patch?
Please do. Jano
The helper converts the 'type', 'format' and index values to enum values/numbers and does validation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 45 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c93c4df929..9b64e33ff5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9039,6 +9039,45 @@ virDomainDiskSourcePRParse(xmlNodePtr node, } +virStorageSourcePtr +virDomainStorageSourceParseBase(const char *type, + const char *format, + const char *index) +{ + VIR_AUTOUNREF(virStorageSourcePtr) src = NULL; + virStorageSourcePtr ret = NULL; + + if (!(src = virStorageSourceNew())) + return NULL; + + src->type = VIR_STORAGE_TYPE_FILE; + + if (type && + (src->type = virStorageTypeFromString(type)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown storage source type '%s'"), type); + return NULL; + } + + if (format && + (src->format = virStorageFileFormatTypeFromString(format)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown storage source format '%s'"), format); + return NULL; + } + + if (index && + virStrToLong_uip(index, NULL, 10, &src->id) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid storage source index '%s'"), index); + return NULL; + } + + VIR_STEAL_PTR(ret, src); + return ret; +} + + int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f550c257d6..ce6e5b4748 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3452,6 +3452,11 @@ int virDomainDiskDefCheckDuplicateInfo(const virDomainDiskDef *a, const virDomainDiskDef *b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +virStorageSourcePtr +virDomainStorageSourceParseBase(const char *type, + const char *format, + const char *index) + ATTRIBUTE_RETURN_CHECK; int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 681ff911b6..6246b4c034 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -565,6 +565,7 @@ virDomainStateTypeFromString; virDomainStateTypeToString; virDomainStorageNetworkParseHost; virDomainStorageSourceParse; +virDomainStorageSourceParseBase; virDomainTaintTypeFromString; virDomainTaintTypeToString; virDomainTimerModeTypeFromString; -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:52PM +0100, Peter Krempa wrote:
The helper converts the 'type', 'format' and index values to enum values/numbers and does validation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 45 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9b64e33ff5..15bb00e800 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9166,31 +9166,15 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, 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); + if (!(src->backingStore = virStorageSourceNew())) + return -1; 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 (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) + idx = virXMLPropString(ctxt->node, "index"); if (!(format = virXPathString("string(./format/@type)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -9198,19 +9182,18 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, return -1; } - backingStore->format = virStorageFileFormatTypeFromString(format); - if (backingStore->format <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk backing store format '%s'"), format); - return -1; - } - if (!(source = virXPathNode("./source", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing disk backing store source")); return -1; } + if (!(backingStore = virDomainStorageSourceParseBase(type, format, idx))) + 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) return -1; -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:53PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 15bb00e800..8fa06aff03 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9078,6 +9078,18 @@ virDomainStorageSourceParseBase(const char *type, } +/** + * virDomainStorageSourceParse: + * @node: XML node pointing to the source element to parse + * @ctxt: XPath context + * @src: filled with parsed data + * @flags: XML parser flags + * @xmlopt: XML parser callbacks + * + * Parses @src definition from element pointed to by @node. Note that this + * does not parse the 'type' and 'format' attributes of @src and 'type' needs + * to be set correctly prior to calling this function. + */ int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:54PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Pass in 'src' rather than the backing store of it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8fa06aff03..5773d07474 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23840,13 +23840,14 @@ virDomainDiskSourceFormat(virBufferPtr buf, static int virDomainDiskBackingStoreFormat(virBufferPtr buf, - virStorageSourcePtr backingStore, + virStorageSourcePtr src, 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; + virStorageSourcePtr backingStore = src->backingStore; virBufferSetChildIndent(&childBuf, buf); @@ -23879,8 +23880,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, if (virDomainDiskSourceFormat(&childBuf, backingStore, 0, false, flags, xmlopt) < 0) return -1; - if (virDomainDiskBackingStoreFormat(&childBuf, backingStore->backingStore, - xmlopt, flags) < 0) + if (virDomainDiskBackingStoreFormat(&childBuf, backingStore, xmlopt, flags) < 0) return -1; if (virXMLFormatElement(buf, "backingStore", &attrBuf, &childBuf) < 0) @@ -24142,8 +24142,7 @@ virDomainDiskDefFormat(virBufferPtr buf, /* 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) + if (virDomainDiskBackingStoreFormat(buf, def->src, xmlopt, flags) < 0) return -1; virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:55PM +0100, Peter Krempa wrote:
Pass in 'src' rather than the backing store of it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
If NULL is passed, the function will lookup <source> in current context. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5773d07474..852489e185 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9080,7 +9080,7 @@ virDomainStorageSourceParseBase(const char *type, /** * virDomainStorageSourceParse: - * @node: XML node pointing to the source element to parse + * @node: XML node pointing to the source element to parse (see below) * @ctxt: XPath context * @src: filled with parsed data * @flags: XML parser flags @@ -9089,6 +9089,9 @@ virDomainStorageSourceParseBase(const char *type, * Parses @src definition from element pointed to by @node. Note that this * does not parse the 'type' and 'format' attributes of @src and 'type' needs * to be set correctly prior to calling this function. + * + * If @node is NULL a <source> subelement is looked up in @ctxt to be used as + * source. Error is reported if the source is not found. */ int virDomainStorageSourceParse(xmlNodePtr node, @@ -9100,6 +9103,13 @@ virDomainStorageSourceParse(xmlNodePtr node, VIR_XPATH_NODE_AUTORESTORE(ctxt); xmlNodePtr tmp; + if (!node && + !(node = virXPathNode("./source", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing <source> element for storage source")); + return -1; + } + ctxt->node = node; switch ((virStorageType)src->type) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce6e5b4748..6fb73bdaf7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3463,7 +3463,7 @@ int virDomainStorageSourceParse(xmlNodePtr node, virStorageSourcePtr src, unsigned int flags, virDomainXMLOptionPtr xmlopt) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, int maplen, -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:56PM +0100, Peter Krempa wrote:
If NULL is passed, the function will lookup <source> in current context.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5773d07474..852489e185 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9080,7 +9080,7 @@ virDomainStorageSourceParseBase(const char *type,
/** * virDomainStorageSourceParse: - * @node: XML node pointing to the source element to parse + * @node: XML node pointing to the source element to parse (see below) * @ctxt: XPath context * @src: filled with parsed data * @flags: XML parser flags @@ -9089,6 +9089,9 @@ virDomainStorageSourceParseBase(const char *type, * Parses @src definition from element pointed to by @node. Note that this * does not parse the 'type' and 'format' attributes of @src and 'type' needs * to be set correctly prior to calling this function. + * + * If @node is NULL a <source> subelement is looked up in @ctxt to be used as + * source. Error is reported if the source is not found. */ int virDomainStorageSourceParse(xmlNodePtr node, @@ -9100,6 +9103,13 @@ virDomainStorageSourceParse(xmlNodePtr node, VIR_XPATH_NODE_AUTORESTORE(ctxt); xmlNodePtr tmp;
+ if (!node && + !(node = virXPathNode("./source", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing <source> element for storage source")); + return -1; + } +
This feels odd to me and there is only one caller using this at the end of the series. Most of the callers already have a node handy and there's the migrationSource vs. source where the 'source' node is preferred and optional. I'd rather open-code the lookup in DiskDefMirrorParse Jano
ctxt->node = node;
switch ((virStorageType)src->type) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce6e5b4748..6fb73bdaf7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3463,7 +3463,7 @@ int virDomainStorageSourceParse(xmlNodePtr node, virStorageSourcePtr src, unsigned int flags, virDomainXMLOptionPtr xmlopt) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, int maplen, -- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3b0887a194..9323421e49 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2715,8 +2715,8 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, { VIR_XPATH_NODE_AUTORESTORE(ctxt); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - char *format = NULL; - char *type = NULL; + VIR_AUTOFREE(char *) format = NULL; + VIR_AUTOFREE(char *) type = NULL; int ret = -1; VIR_AUTOUNREF(virStorageSourcePtr) migrSource = NULL; xmlNodePtr sourceNode; @@ -2770,8 +2770,6 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, ret = 0; cleanup: - VIR_FREE(format); - VIR_FREE(type); return ret; } -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:57PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9323421e49..c7454ce821 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2717,42 +2717,39 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); VIR_AUTOFREE(char *) format = NULL; VIR_AUTOFREE(char *) type = NULL; - int ret = -1; VIR_AUTOUNREF(virStorageSourcePtr) migrSource = NULL; xmlNodePtr sourceNode; ctxt->node = node; - if (!(ctxt->node = virXPathNode("./migrationSource", ctxt))) { - ret = 0; - goto cleanup; - } + if (!(ctxt->node = virXPathNode("./migrationSource", ctxt))) + return 0; if (!(migrSource = virStorageSourceNew())) - goto cleanup; + return -1; if (!(type = virXMLPropString(ctxt->node, "type"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage source type")); - goto cleanup; + return -1; } if (!(format = virXMLPropString(ctxt->node, "format"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage source format")); - goto cleanup; + return -1; } if ((migrSource->type = virStorageTypeFromString(type)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown storage source type '%s'"), type); - goto cleanup; + return -1; } if ((migrSource->format = virStorageFileFormatTypeFromString(format)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown storage source format '%s'"), format); - goto cleanup; + return -1; } if ((sourceNode = virXPathNode("./source", ctxt))) @@ -2760,17 +2757,14 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, if (virDomainStorageSourceParse(ctxt->node, ctxt, migrSource, VIR_DOMAIN_DEF_PARSE_STATUS, NULL) < 0) - goto cleanup; + return -1; if ((ctxt->node = virXPathNode("./privateData", ctxt)) && qemuStorageSourcePrivateDataParse(ctxt, migrSource) < 0) - goto cleanup; + return -1; VIR_STEAL_PTR(diskPriv->migrSource, migrSource); - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:58PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The slight possible regression in error message descriptivness doesn't matter much as the function parses private data which should not be touched by users in the first place. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c7454ce821..0c06e3b23a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2725,32 +2725,15 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, if (!(ctxt->node = virXPathNode("./migrationSource", ctxt))) return 0; - if (!(migrSource = virStorageSourceNew())) - return -1; - - if (!(type = virXMLPropString(ctxt->node, "type"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage source type")); - return -1; - } - - if (!(format = virXMLPropString(ctxt->node, "format"))) { + if (!(type = virXMLPropString(ctxt->node, "type")) || + !(format = virXMLPropString(ctxt->node, "format"))) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage source format")); + _("missing NBD migration storage source type or format")); return -1; } - if ((migrSource->type = virStorageTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage source type '%s'"), type); - return -1; - } - - if ((migrSource->format = virStorageFileFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage source format '%s'"), format); + if (!(migrSource = virDomainStorageSourceParseBase(type, format, NULL))) return -1; - } if ((sourceNode = virXPathNode("./source", ctxt))) ctxt->node = sourceNode; -- 2.20.1
On Fri, Mar 22, 2019 at 07:00:59PM +0100, Peter Krempa wrote:
The slight possible regression in error message descriptivness doesn't matter much as the function parses private data which should not be touched by users in the first place.
Joining the error messages here is not necessary to use virDomainStorageSourceParseBase and therefore does not belong in this commit. Moreover, hereunder, if the error messages do not matter in practice I suggest leaving them untouched to save the translators some work.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c7454ce821..0c06e3b23a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2725,32 +2725,15 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, if (!(ctxt->node = virXPathNode("./migrationSource", ctxt))) return 0;
- if (!(migrSource = virStorageSourceNew())) - return -1; -
- if (!(type = virXMLPropString(ctxt->node, "type"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage source type")); - return -1; - } - - if (!(format = virXMLPropString(ctxt->node, "format"))) { + if (!(type = virXMLPropString(ctxt->node, "type")) || + !(format = virXMLPropString(ctxt->node, "format"))) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage source format")); + _("missing NBD migration storage source type or format")); return -1; }
Wit this hunk left alone: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
- if ((migrSource->type = virStorageTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage source type '%s'"), type); - return -1; - } - - if ((migrSource->format = virStorageFileFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage source format '%s'"), format); + if (!(migrSource = virDomainStorageSourceParseBase(type, format, NULL))) return -1; - }
if ((sourceNode = virXPathNode("./source", ctxt))) ctxt->node = sourceNode; -- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Drop the local call in favor of passing in xmlopt. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0c06e3b23a..0be00f804e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2711,7 +2711,8 @@ qemuDomainObjPrivateXMLParsePR(xmlXPathContextPtr ctxt, static int qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, xmlXPathContextPtr ctxt, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + virDomainXMLOptionPtr xmlopt) { VIR_XPATH_NODE_AUTORESTORE(ctxt); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); @@ -2739,11 +2740,7 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, ctxt->node = sourceNode; if (virDomainStorageSourceParse(ctxt->node, ctxt, migrSource, - VIR_DOMAIN_DEF_PARSE_STATUS, NULL) < 0) - return -1; - - if ((ctxt->node = virXPathNode("./privateData", ctxt)) && - qemuStorageSourcePrivateDataParse(ctxt, migrSource) < 0) + VIR_DOMAIN_DEF_PARSE_STATUS, xmlopt) < 0) return -1; VIR_STEAL_PTR(diskPriv->migrSource, migrSource); @@ -2779,7 +2776,8 @@ qemuDomainObjPrivateXMLParseJobNBD(virDomainObjPtr vm, QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating = true; if (qemuDomainObjPrivateXMLParseJobNBDSource(nodes[i], ctxt, - disk) < 0) + disk, + priv->driver->xmlopt) < 0) goto cleanup; } -- 2.20.1
On Fri, Mar 22, 2019 at 07:01:00PM +0100, Peter Krempa wrote:
Drop the local call in favor of passing in xmlopt.
Aye
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 852489e185..5c82ac0a18 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24017,8 +24017,12 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, unsigned int flags, virDomainXMLOptionPtr xmlopt) { + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; const char *formatStr = NULL; + virBufferSetChildIndent(&childBuf, buf); + /* For now, mirroring is currently output-only: we only output it * for live domains, therefore we ignore it on input except for * the internal parse on libvirtd restart. We prefer to output @@ -24032,28 +24036,25 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, if (disk->mirror->format) formatStr = virStorageFileFormatTypeToString(disk->mirror->format); - virBufferAsprintf(buf, "<mirror type='%s'", + virBufferAsprintf(&attrBuf, " type='%s'", virStorageTypeToString(disk->mirror->type)); if (disk->mirror->type == VIR_STORAGE_TYPE_FILE && disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { - virBufferEscapeString(buf, " file='%s'", disk->mirror->path); - virBufferEscapeString(buf, " format='%s'", formatStr); + virBufferEscapeString(&attrBuf, " file='%s'", disk->mirror->path); + virBufferEscapeString(&attrBuf, " format='%s'", formatStr); } - virBufferEscapeString(buf, " job='%s'", + virBufferEscapeString(&attrBuf, " job='%s'", virDomainBlockJobTypeToString(disk->mirrorJob)); - if (disk->mirrorState) { - const char *mirror; + if (disk->mirrorState) + virBufferEscapeString(&attrBuf, " ready='%s'", + virDomainDiskMirrorStateTypeToString(disk->mirrorState)); - mirror = virDomainDiskMirrorStateTypeToString(disk->mirrorState); - virBufferEscapeString(buf, " ready='%s'", mirror); - } - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(buf, disk->mirror, 0, false, 0, xmlopt) < 0) + virBufferEscapeString(&childBuf, "<format type='%s'/>\n", formatStr); + if (virDomainDiskSourceFormat(&childBuf, disk->mirror, 0, false, 0, xmlopt) < 0) + return -1; + + if (virXMLFormatElement(buf, "mirror", &attrBuf, &childBuf) < 0) return -1; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</mirror>\n"); return 0; } -- 2.20.1
On Fri, Mar 22, 2019 at 07:01:01PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We have the proper flags available so we can pass them to the fomatter. The added bonus is that private data may be formatted into the status XML. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5c82ac0a18..40924b1d29 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24050,7 +24050,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virDomainDiskMirrorStateTypeToString(disk->mirrorState)); virBufferEscapeString(&childBuf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(&childBuf, disk->mirror, 0, false, 0, xmlopt) < 0) + if (virDomainDiskSourceFormat(&childBuf, disk->mirror, 0, false, flags, xmlopt) < 0) return -1; if (virXMLFormatElement(buf, "mirror", &attrBuf, &childBuf) < 0) -- 2.20.1
On Fri, Mar 22, 2019 at 07:01:02PM +0100, Peter Krempa wrote:
We have the proper flags available so we can pass them to the fomatter. The added bonus is that private data may be formatted into the status XML.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use virDomainStorageSourceParseBase and other tricks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 52 +++++++++++++----------------------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40924b1d29..2514c60744 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9315,14 +9315,13 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, unsigned int flags, virDomainXMLOptionPtr xmlopt) { - xmlNodePtr mirrorNode; + VIR_XPATH_NODE_AUTORESTORE(ctxt); 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; + ctxt->node = cur; if ((blockJob = virXMLPropString(cur, "job"))) { if ((def->mirrorJob = virDomainBlockJobTypeFromString(blockJob)) <= 0) { @@ -9335,35 +9334,8 @@ 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); - 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")); - return -1; - } - - if (virDomainStorageSourceParse(mirrorNode, ctxt, def->mirror, - flags, xmlopt) < 0) - return -1; + mirrorFormat = virXPathString("string(./format/@type)", ctxt); } else { - /* For back-compat reasons, we handle a file name - * encoded as attributes, even though we prefer - * modern output in the style of backingStore */ - def->mirror->type = VIR_STORAGE_TYPE_FILE; - def->mirror->path = virXMLPropString(cur, "file"); - if (!def->mirror->path) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("mirror requires file name")); - return -1; - } if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { virReportError(VIR_ERR_XML_ERROR, "%s", _("mirror without type only supported " @@ -9373,11 +9345,19 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, mirrorFormat = virXMLPropString(cur, "format"); } - if (mirrorFormat) { - def->mirror->format = virStorageFileFormatTypeFromString(mirrorFormat); - if (def->mirror->format <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown mirror format value '%s'"), mirrorFormat); + if (!(def->mirror = virDomainStorageSourceParseBase(mirrorType, mirrorFormat, NULL))) + return -1; + + if (mirrorType) { + if (virDomainStorageSourceParse(NULL, 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 */ + if (!(def->mirror->path = virXMLPropString(cur, "file"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("mirror requires file name")); return -1; } } -- 2.20.1
On Fri, Mar 22, 2019 at 07:01:03PM +0100, Peter Krempa wrote:
Use virDomainStorageSourceParseBase and other tricks.
Would be nicer to read with the virDomainStorageSourceParseBase trick and the virDomainStorageSourceParse trick being separate.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 52 +++++++++++++----------------------------- 1 file changed, 16 insertions(+), 36 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40924b1d29..2514c60744 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9315,14 +9315,13 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, unsigned int flags, virDomainXMLOptionPtr xmlopt) { - xmlNodePtr mirrorNode; + VIR_XPATH_NODE_AUTORESTORE(ctxt); 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; + ctxt->node = cur;
if ((blockJob = virXMLPropString(cur, "job"))) { if ((def->mirrorJob = virDomainBlockJobTypeFromString(blockJob)) <= 0) { @@ -9335,35 +9334,8 @@ 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); - 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")); - return -1; - } -
I'd move this
- if (virDomainStorageSourceParse(mirrorNode, ctxt, def->mirror, - flags, xmlopt) < 0) - return -1; + mirrorFormat = virXPathString("string(./format/@type)", ctxt); } else { - /* For back-compat reasons, we handle a file name - * encoded as attributes, even though we prefer - * modern output in the style of backingStore */ - def->mirror->type = VIR_STORAGE_TYPE_FILE; - def->mirror->path = virXMLPropString(cur, "file"); - if (!def->mirror->path) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("mirror requires file name")); - return -1; - } if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { virReportError(VIR_ERR_XML_ERROR, "%s", _("mirror without type only supported " @@ -9373,11 +9345,19 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, mirrorFormat = virXMLPropString(cur, "format"); }
- if (mirrorFormat) { - def->mirror->format = virStorageFileFormatTypeFromString(mirrorFormat); - if (def->mirror->format <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown mirror format value '%s'"), mirrorFormat); + if (!(def->mirror = virDomainStorageSourceParseBase(mirrorType, mirrorFormat, NULL))) + return -1; + + if (mirrorType) {
here.
+ if (virDomainStorageSourceParse(NULL, 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 */ + if (!(def->mirror->path = virXMLPropString(cur, "file"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("mirror requires file name")); return -1;
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 | 5 +++++ tests/qemustatusxml2xmldata/blockjob-mirror-in.xml | 13 +++++++++++++ tests/qemuxml2argvdata/disk-mirror.xml | 6 ++++++ tests/qemuxml2xmloutdata/disk-mirror-active.xml | 6 ++++++ 5 files changed, 31 insertions(+) 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 2514c60744..0970d48045 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9351,6 +9351,8 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, if (mirrorType) { if (virDomainStorageSourceParse(NULL, ctxt, def->mirror, flags, xmlopt) < 0) return -1; + if (virDomainDiskBackingStoreParse(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 @@ -24033,6 +24035,9 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, if (virDomainDiskSourceFormat(&childBuf, disk->mirror, 0, false, flags, xmlopt) < 0) return -1; + if (virDomainDiskBackingStoreFormat(&childBuf, disk->mirror, xmlopt, flags) < 0) + return -1; + if (virXMLFormatElement(buf, "mirror", &attrBuf, &childBuf) < 0) return -1; 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 Fri, Mar 22, 2019 at 07:01:04PM +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 | 5 +++++ tests/qemustatusxml2xmldata/blockjob-mirror-in.xml | 13 +++++++++++++ tests/qemuxml2argvdata/disk-mirror.xml | 6 ++++++ tests/qemuxml2xmloutdata/disk-mirror-active.xml | 6 ++++++ 5 files changed, 31 insertions(+)
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 | 7 +++++-- tests/qemuxml2argvdata/disk-mirror.xml | 4 ++-- tests/qemuxml2xmloutdata/disk-mirror-active.xml | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0970d48045..72dd45feb8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9320,6 +9320,7 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, VIR_AUTOFREE(char *) mirrorType = NULL; VIR_AUTOFREE(char *) ready = NULL; VIR_AUTOFREE(char *) blockJob = NULL; + VIR_AUTOFREE(char *) index = NULL; ctxt->node = cur; @@ -9335,6 +9336,7 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, if ((mirrorType = virXMLPropString(cur, "type"))) { mirrorFormat = virXPathString("string(./format/@type)", ctxt); + index = virXPathString("string(./source/@index)", ctxt); } else { if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -9345,7 +9347,8 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def, mirrorFormat = virXMLPropString(cur, "format"); } - if (!(def->mirror = virDomainStorageSourceParseBase(mirrorType, mirrorFormat, NULL))) + if (!(def->mirror = virDomainStorageSourceParseBase(mirrorType, mirrorFormat, + index))) return -1; if (mirrorType) { @@ -24032,7 +24035,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virDomainDiskMirrorStateTypeToString(disk->mirrorState)); virBufferEscapeString(&childBuf, "<format type='%s'/>\n", formatStr); - if (virDomainDiskSourceFormat(&childBuf, disk->mirror, 0, false, flags, xmlopt) < 0) + if (virDomainDiskSourceFormat(&childBuf, disk->mirror, 0, true, flags, xmlopt) < 0) return -1; if (virDomainDiskBackingStoreFormat(&childBuf, disk->mirror, xmlopt, flags) < 0) 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 Fri, Mar 22, 2019 at 07:01:05PM +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 | 7 +++++-- tests/qemuxml2argvdata/disk-mirror.xml | 4 ++-- tests/qemuxml2xmloutdata/disk-mirror-active.xml | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa