[PATCH 0/7] qemu: Fix hot-(un-)plug of disks with FD-passed storage
Peter Krempa (7): qemu_fd: Remove declaration for 'qemuFDPassNewDirect' qemuStorageSourcePrivateDataFormat: Rename 'tmp' to 'objectsChildBuf' qemu: command: Handle FD passing commandline via qemuBuildBlockStorageSourceAttachDataCommandline qemuFDPassTransferCommand: Mark that FD was passed qemu: fd: Add helpers allowing storing FD set data in status XML qemu: domain: Store fdset ID for disks passed to qemu via FD qemu: block: Properly handle FD-passed disk hot-(un-)plug src/qemu/qemu_block.c | 7 +++ src/qemu/qemu_block.h | 2 + src/qemu/qemu_command.c | 26 ++--------- src/qemu/qemu_domain.c | 31 +++++++++---- src/qemu/qemu_fd.c | 43 +++++++++++++++++++ src/qemu/qemu_fd.h | 8 +++- tests/qemustatusxml2xmldata/modern-in.xml | 3 ++ .../disk-source-fd.x86_64-latest.args | 6 +-- 8 files changed, 91 insertions(+), 35 deletions(-) -- 2.39.1
The function doesn't exist any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_fd.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_fd.h b/src/qemu/qemu_fd.h index 6f165b6be9..032b9442ee 100644 --- a/src/qemu/qemu_fd.h +++ b/src/qemu/qemu_fd.h @@ -30,9 +30,6 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuFDPass, qemuFDPassFree); qemuFDPass * qemuFDPassNew(const char *prefix, void *dompriv); -qemuFDPass * -qemuFDPassNewDirect(const char *prefix, - void *dompriv); void qemuFDPassAddFD(qemuFDPass *fdpass, -- 2.39.1
Be consistent with other children buffer variable naming scheme. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c331c2e7be..112d4fb90e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2005,9 +2005,9 @@ static int qemuStorageSourcePrivateDataFormat(virStorageSource *src, virBuffer *buf) { - g_auto(virBuffer) tmp = VIR_BUFFER_INIT_CHILD(buf); qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); g_auto(virBuffer) nodenamesChildBuf = VIR_BUFFER_INIT_CHILD(buf); + g_auto(virBuffer) objectsChildBuf = VIR_BUFFER_INIT_CHILD(buf); virBufferEscapeString(&nodenamesChildBuf, "<nodename type='storage' name='%s'/>\n", src->nodestorage); virBufferEscapeString(&nodenamesChildBuf, "<nodename type='format' name='%s'/>\n", src->nodeformat); @@ -2025,16 +2025,16 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src, return -1; if (srcPriv) { - qemuStorageSourcePrivateDataFormatSecinfo(&tmp, srcPriv->secinfo, "auth"); - qemuStorageSourcePrivateDataFormatSecinfo(&tmp, srcPriv->encinfo, "encryption"); - qemuStorageSourcePrivateDataFormatSecinfo(&tmp, srcPriv->httpcookie, "httpcookie"); - qemuStorageSourcePrivateDataFormatSecinfo(&tmp, srcPriv->tlsKeySecret, "tlskey"); + qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->secinfo, "auth"); + qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->encinfo, "encryption"); + qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->httpcookie, "httpcookie"); + qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->tlsKeySecret, "tlskey"); } if (src->tlsAlias) - virBufferAsprintf(&tmp, "<TLSx509 alias='%s'/>\n", src->tlsAlias); + virBufferAsprintf(&objectsChildBuf, "<TLSx509 alias='%s'/>\n", src->tlsAlias); - virXMLFormatElement(buf, "objects", NULL, &tmp); + virXMLFormatElement(buf, "objects", NULL, &objectsChildBuf); if (src->thresholdEventWithIndex) virBufferAddLit(buf, "<thresholdEvent indexUsed='yes'/>\n"); -- 2.39.1
Copy the pointer to qemuFDPass into struct qemuBlockStorageSourceAttachData so that it can be used from qemuBuildBlockStorageSourceAttachDataCommandline rather than looping again in qemuBuildDiskSourceCommandLineFDs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.h | 2 ++ src/qemu/qemu_command.c | 26 +++---------------- .../disk-source-fd.x86_64-latest.args | 6 ++--- 3 files changed, 9 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index eac986e0f0..5a61a19da2 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -99,6 +99,8 @@ struct qemuBlockStorageSourceAttachData { char *tlsAlias; virJSONValue *tlsKeySecretProps; char *tlsKeySecretAlias; + + qemuFDPass *fdpass; }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 297f53ba84..90dc6b5434 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2125,6 +2125,8 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd, return -1; } + qemuFDPassTransferCommand(data->fdpass, cmd); + if (data->storageProps) { if (!(tmp = virJSONValueToString(data->storageProps, false))) return -1; @@ -2153,25 +2155,6 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd, } -static int -qemuBuildDiskSourceCommandLineFDs(virCommand *cmd, - virDomainDiskDef *disk) -{ - virStorageSource *n; - - for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { - qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(n); - - if (!srcpriv || !srcpriv->fdpass) - continue; - - qemuFDPassTransferCommand(srcpriv->fdpass, cmd); - } - - return 0; -} - - static int qemuBuildDiskSourceCommandLine(virCommand *cmd, virDomainDiskDef *disk, @@ -2189,9 +2172,6 @@ qemuBuildDiskSourceCommandLine(virCommand *cmd, if (virStorageSourceIsEmpty(disk->src)) return 0; - if (qemuBuildDiskSourceCommandLineFDs(cmd, disk) < 0) - return -1; - if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src))) return -1; @@ -10654,6 +10634,8 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSource *src, tlsKeySecretAlias = srcpriv->tlsKeySecret->alias; } + + data->fdpass = srcpriv->fdpass; } if (src->haveTLS == VIR_TRISTATE_BOOL_YES && diff --git a/tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args b/tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args index b4a81acfc7..a7ddd65000 100644 --- a/tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args +++ b/tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args @@ -33,13 +33,13 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -blockdev '{"driver":"file","filename":"/dev/fdset/2","node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-4-format","read-only":false,"driver":"qcow2","file":"libvirt-4-storage"}' \ -device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-4-format","id":"virtio-disk4","bootindex":1}' \ --add-fd set=0,fd=704,opaque=libvirt-1-storage0 \ --add-fd set=1,fd=777,opaque=libvirt-2-storage0 \ --add-fd set=1,fd=778,opaque=libvirt-2-storage1 \ -blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/rhel7.1484071876","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-3-format","read-only":true,"driver":"qcow2","file":"libvirt-3-storage","backing":null}' \ +-add-fd set=1,fd=777,opaque=libvirt-2-storage0 \ +-add-fd set=1,fd=778,opaque=libvirt-2-storage1 \ -blockdev '{"driver":"file","filename":"/dev/fdset/1","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"qcow2","file":"libvirt-2-storage","backing":"libvirt-3-format"}' \ +-add-fd set=0,fd=704,opaque=libvirt-1-storage0 \ -blockdev '{"driver":"file","filename":"/dev/fdset/0","node-name":"libvirt-1-storage","read-only":false,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage","backing":"libvirt-2-format"}' \ -device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-1-format","id":"virtio-disk5"}' \ -- 2.39.1
Until now the code didn't expect that we'd want to rollback/detach a FD passed on the commandline, but whith disk backend FD passing this can happen. Properly mark the 'qemuFDPass' object as passed to qemu even when it was done on the commandline. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_fd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_fd.c b/src/qemu/qemu_fd.c index 51a8133fde..ebeeb65505 100644 --- a/src/qemu/qemu_fd.c +++ b/src/qemu/qemu_fd.c @@ -151,6 +151,8 @@ qemuFDPassTransferCommand(qemuFDPass *fdpass, fdpass->fds[i].fd = -1; virCommandAddArgList(cmd, "-add-fd", arg, NULL); } + + fdpass->passed = true; } -- 2.39.1
Rollback of FD sets passed to qemu is also needed after possible restart of libvirtd when we need to serialize the data into status XML. For this purpose we need to access the fdset ID once it was passed to qemu and potentially re-create a 'qemuFDPass' struct in passed state. Introduce 'qemuFDPassNewPassed' and 'qemuFDPassIsPassed'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_fd.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_fd.h | 7 +++++++ 2 files changed, 48 insertions(+) diff --git a/src/qemu/qemu_fd.c b/src/qemu/qemu_fd.c index ebeeb65505..9eaaa098ee 100644 --- a/src/qemu/qemu_fd.c +++ b/src/qemu/qemu_fd.c @@ -96,6 +96,47 @@ qemuFDPassNew(const char *prefix, } +/** + * qemuFDPassNewPassed: + * @fdSetID: ID of an FDset which was allready passed to qemu + * + * Create qemuFDPass pointing to an already passed FD. Useful to usw with + * qemuFDPassTransferMonitorRollback, when restoring after restart. + */ +qemuFDPass * +qemuFDPassNewPassed(unsigned int fdSetID) +{ + qemuFDPass *fdpass = g_new0(qemuFDPass, 1); + + fdpass->fdSetID = fdSetID; + fdpass->passed = true; + + return fdpass; +} + + +/** + * qemuFDPassIsPassed: + * @fdpass: The fd passing helper struct + * @id: when non-NULL filled with the fdset ID + * + * Returns true if @fdpass was passed to qemu. In such case @id is also filled + * with the ID of the fdset if non-NULL. + */ +bool +qemuFDPassIsPassed(qemuFDPass *fdpass, + unsigned *id) +{ + if (!fdpass) + return false; + + if (id) + *id = fdpass->fdSetID; + + return true; +} + + /** * qemuFDPassAddFD: * @fdpass: The fd passing helper struct diff --git a/src/qemu/qemu_fd.h b/src/qemu/qemu_fd.h index 032b9442ee..cd0ff2c690 100644 --- a/src/qemu/qemu_fd.h +++ b/src/qemu/qemu_fd.h @@ -31,6 +31,13 @@ qemuFDPass * qemuFDPassNew(const char *prefix, void *dompriv); +qemuFDPass * +qemuFDPassNewPassed(unsigned int fdSetID); + +bool +qemuFDPassIsPassed(qemuFDPass *fdpass, + unsigned *id); + void qemuFDPassAddFD(qemuFDPass *fdpass, int *fd, -- 2.39.1
On Tue, Jan 31, 2023 at 05:32:11PM +0100, Peter Krempa wrote:
Rollback of FD sets passed to qemu is also needed after possible restart of libvirtd when we need to serialize the data into status XML. For this purpose we need to access the fdset ID once it was passed to qemu and potentially re-create a 'qemuFDPass' struct in passed state.
Introduce 'qemuFDPassNewPassed' and 'qemuFDPassIsPassed'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_fd.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_fd.h | 7 +++++++ 2 files changed, 48 insertions(+)
diff --git a/src/qemu/qemu_fd.c b/src/qemu/qemu_fd.c index ebeeb65505..9eaaa098ee 100644 --- a/src/qemu/qemu_fd.c +++ b/src/qemu/qemu_fd.c @@ -96,6 +96,47 @@ qemuFDPassNew(const char *prefix, }
+/** + * qemuFDPassNewPassed: + * @fdSetID: ID of an FDset which was allready passed to qemu + * + * Create qemuFDPass pointing to an already passed FD. Useful to usw with + * qemuFDPassTransferMonitorRollback, when restoring after restart. + */ +qemuFDPass * +qemuFDPassNewPassed(unsigned int fdSetID) +{ + qemuFDPass *fdpass = g_new0(qemuFDPass, 1); + + fdpass->fdSetID = fdSetID; + fdpass->passed = true; + + return fdpass; +} + + +/** + * qemuFDPassIsPassed: + * @fdpass: The fd passing helper struct + * @id: when non-NULL filled with the fdset ID + * + * Returns true if @fdpass was passed to qemu. In such case @id is also filled + * with the ID of the fdset if non-NULL. + */ +bool +qemuFDPassIsPassed(qemuFDPass *fdpass, + unsigned *id) +{ + if (!fdpass) + return false;
Shouldn't this rather be like this? if (!fdpass || !fdpass->passed) return false;
+ + if (id) + *id = fdpass->fdSetID; + + return true; +} + + /** * qemuFDPassAddFD: * @fdpass: The fd passing helper struct diff --git a/src/qemu/qemu_fd.h b/src/qemu/qemu_fd.h index 032b9442ee..cd0ff2c690 100644 --- a/src/qemu/qemu_fd.h +++ b/src/qemu/qemu_fd.h @@ -31,6 +31,13 @@ qemuFDPass * qemuFDPassNew(const char *prefix, void *dompriv);
+qemuFDPass * +qemuFDPassNewPassed(unsigned int fdSetID); + +bool +qemuFDPassIsPassed(qemuFDPass *fdpass, + unsigned *id); + void qemuFDPassAddFD(qemuFDPass *fdpass, int *fd, -- 2.39.1
To ensure that we can hot-unplug the disk including the associated fdset we need to store the fdset ID in the status XML. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 17 ++++++++++++++++- tests/qemustatusxml2xmldata/modern-in.xml | 3 +++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 112d4fb90e..1757a6aaad 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1941,6 +1941,8 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, g_autofree char *httpcookiealias = NULL; g_autofree char *tlskeyalias = NULL; g_autofree char *thresholdEventWithIndex = NULL; + bool fdsetPresent = false; + unsigned int fdSetID; src->nodestorage = virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt); src->nodeformat = virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt); @@ -1957,7 +1959,9 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, httpcookiealias = virXPathString("string(./objects/secret[@type='httpcookie']/@alias)", ctxt); tlskeyalias = virXPathString("string(./objects/secret[@type='tlskey']/@alias)", ctxt); - if (authalias || encalias || httpcookiealias || tlskeyalias) { + fdsetPresent = virXPathUInt("string(./fdsets/fdset[@type='storage']/@id)", ctxt, &fdSetID) == 0; + + if (authalias || encalias || httpcookiealias || tlskeyalias || fdsetPresent) { if (!src->privateData && !(src->privateData = qemuDomainStorageSourcePrivateNew())) return -1; @@ -1975,6 +1979,9 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->tlsKeySecret, &tlskeyalias) < 0) return -1; + + if (fdsetPresent) + priv->fdpass = qemuFDPassNewPassed(fdSetID); } if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0) @@ -2008,6 +2015,7 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src, qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); g_auto(virBuffer) nodenamesChildBuf = VIR_BUFFER_INIT_CHILD(buf); g_auto(virBuffer) objectsChildBuf = VIR_BUFFER_INIT_CHILD(buf); + g_auto(virBuffer) fdsetsChildBuf = VIR_BUFFER_INIT_CHILD(buf); virBufferEscapeString(&nodenamesChildBuf, "<nodename type='storage' name='%s'/>\n", src->nodestorage); virBufferEscapeString(&nodenamesChildBuf, "<nodename type='format' name='%s'/>\n", src->nodeformat); @@ -2025,10 +2033,15 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src, return -1; if (srcPriv) { + unsigned int fdSetID; + qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->secinfo, "auth"); qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->encinfo, "encryption"); qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->httpcookie, "httpcookie"); qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->tlsKeySecret, "tlskey"); + + if (qemuFDPassIsPassed(srcPriv->fdpass, &fdSetID)) + virBufferAsprintf(&fdsetsChildBuf, "<fdset type='storage' id='%u'/>\n", fdSetID); } if (src->tlsAlias) @@ -2036,6 +2049,8 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src, virXMLFormatElement(buf, "objects", NULL, &objectsChildBuf); + virXMLFormatElement(buf, "fdsets", NULL, &fdsetsChildBuf); + if (src->thresholdEventWithIndex) virBufferAddLit(buf, "<thresholdEvent indexUsed='yes'/>\n"); diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml index 7759034f7a..f5beab722b 100644 --- a/tests/qemustatusxml2xmldata/modern-in.xml +++ b/tests/qemustatusxml2xmldata/modern-in.xml @@ -341,6 +341,9 @@ <secret type='tlskey' alias='tls-certificate-key-alias'/> <TLSx509 alias='transport-alias'/> </objects> + <fdsets> + <fdset type='storage' id='1337'/> + </fdsets> <thresholdEvent indexUsed='yes'/> </privateData> </source> -- 2.39.1
The hotplug code paths need to be able to pass the FDs to the monitor to ensure that hotplug works. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index e865aa17f9..c218262691 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1410,6 +1410,9 @@ qemuBlockStorageSourceAttachApplyStorageDeps(qemuMonitor *mon, qemuMonitorAddObject(mon, &data->tlsProps, &data->tlsAlias) < 0) return -1; + if (qemuFDPassTransferMonitor(data->fdpass, mon) < 0) + return -1; + return 0; } @@ -1559,6 +1562,8 @@ qemuBlockStorageSourceAttachRollback(qemuMonitor *mon, if (data->tlsKeySecretAlias) ignore_value(qemuMonitorDelObject(mon, data->tlsKeySecretAlias, false)); + qemuFDPassTransferMonitorRollback(data->fdpass, mon); + virErrorRestore(&orig_err); } @@ -1609,6 +1614,8 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src) if (srcpriv->tlsKeySecret) data->tlsKeySecretAlias = g_strdup(srcpriv->tlsKeySecret->alias); + + data->fdpass = srcpriv->fdpass; } return g_steal_pointer(&data); -- 2.39.1
On Tue, Jan 31, 2023 at 05:32:06PM +0100, Peter Krempa wrote:
Peter Krempa (7): qemu_fd: Remove declaration for 'qemuFDPassNewDirect' qemuStorageSourcePrivateDataFormat: Rename 'tmp' to 'objectsChildBuf' qemu: command: Handle FD passing commandline via qemuBuildBlockStorageSourceAttachDataCommandline qemuFDPassTransferCommand: Mark that FD was passed qemu: fd: Add helpers allowing storing FD set data in status XML qemu: domain: Store fdset ID for disks passed to qemu via FD qemu: block: Properly handle FD-passed disk hot-(un-)plug
With the minor fix in 5/7: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
src/qemu/qemu_block.c | 7 +++ src/qemu/qemu_block.h | 2 + src/qemu/qemu_command.c | 26 ++--------- src/qemu/qemu_domain.c | 31 +++++++++---- src/qemu/qemu_fd.c | 43 +++++++++++++++++++ src/qemu/qemu_fd.h | 8 +++- tests/qemustatusxml2xmldata/modern-in.xml | 3 ++ .../disk-source-fd.x86_64-latest.args | 6 +-- 8 files changed, 91 insertions(+), 35 deletions(-)
-- 2.39.1
participants (2)
-
Martin Kletzander -
Peter Krempa