[PATCH 1/3] qemu: add a 'chain' parameter to nbdkit start/stop
This will allow us to start or stop nbdkit for just a single disk source or for every source in the backing chain. This will be used in following patches. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_extdevice.c | 8 +++--- src/qemu/qemu_hotplug.c | 6 ++--- src/qemu/qemu_nbdkit.c | 51 ++++++++++++++++++++++++++++++--------- src/qemu/qemu_nbdkit.h | 6 +++-- 4 files changed, 51 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index 3cf3867056..ed5976d1f7 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -234,12 +234,12 @@ qemuExtDevicesStart(virQEMUDriver *driver, for (i = 0; i < def->ndisks; i++) { virDomainDiskDef *disk = def->disks[i]; - if (qemuNbdkitStartStorageSource(driver, vm, disk->src) < 0) + if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0) return -1; } if (def->os.loader && def->os.loader->nvram) { - if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram) < 0) + if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram, true) < 0) return -1; } @@ -297,11 +297,11 @@ qemuExtDevicesStop(virQEMUDriver *driver, for (i = 0; i < def->ndisks; i++) { virDomainDiskDef *disk = def->disks[i]; - qemuNbdkitStopStorageSource(disk->src, vm); + qemuNbdkitStopStorageSource(disk->src, vm, true); } if (def->os.loader && def->os.loader->nvram) - qemuNbdkitStopStorageSource(def->os.loader->nvram, vm); + qemuNbdkitStopStorageSource(def->os.loader->nvram, vm, true); } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 31b00e05ca..e67673b762 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1018,7 +1018,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0) goto cleanup; - if (qemuNbdkitStartStorageSource(driver, vm, disk->src) < 0) + if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0) goto cleanup; } @@ -1045,7 +1045,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (virStorageSourceChainHasManagedPR(disk->src)) ignore_value(qemuHotplugRemoveManagedPR(vm, VIR_ASYNC_JOB_NONE)); - qemuNbdkitStopStorageSource(disk->src, vm); + qemuNbdkitStopStorageSource(disk->src, vm, true); } qemuDomainSecretDiskDestroy(disk); qemuDomainCleanupStorageSourceFD(disk->src); @@ -4562,7 +4562,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, qemuHotplugRemoveManagedPR(vm, VIR_ASYNC_JOB_NONE) < 0) goto cleanup; - qemuNbdkitStopStorageSource(disk->src, vm); + qemuNbdkitStopStorageSource(disk->src, vm, true); if (disk->transient) { VIR_DEBUG("Removing transient overlay '%s' of disk '%s'", diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index 1c72b6fe6a..39f9c58a48 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -893,18 +893,34 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps WITHOUT_NBDKIT_UNUSED, } +static int +qemuNbdkitStartStorageSourceOne(virQEMUDriver *driver, + virDomainObj *vm, + virStorageSource *src) +{ + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + + if (priv && priv->nbdkitProcess && + qemuNbdkitProcessStart(priv->nbdkitProcess, vm, driver) < 0) + return -1; + + return 0; +} + + int qemuNbdkitStartStorageSource(virQEMUDriver *driver, virDomainObj *vm, - virStorageSource *src) + virStorageSource *src, + bool chain) { virStorageSource *backing; - for (backing = src; backing != NULL; backing = backing->backingStore) { - qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(backing); + if (!chain) + return qemuNbdkitStartStorageSourceOne(driver, vm, src); - if (priv && priv->nbdkitProcess && - qemuNbdkitProcessStart(priv->nbdkitProcess, vm, driver) < 0) + for (backing = src; backing != NULL; backing = backing->backingStore) { + if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) return -1; } @@ -912,18 +928,31 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver, } +static void +qemuNbdkitStopStorageSourceOne(virStorageSource *src, + virDomainObj *vm) +{ + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + + if (priv && priv->nbdkitProcess && + qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0) + VIR_WARN("Unable to stop nbdkit for storage source '%s'", + qemuBlockStorageSourceGetStorageNodename(src)); +} + + void qemuNbdkitStopStorageSource(virStorageSource *src, - virDomainObj *vm) + virDomainObj *vm, + bool chain) { virStorageSource *backing; - for (backing = src; backing != NULL; backing = backing->backingStore) { - qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(backing); + if (! chain) + return qemuNbdkitStopStorageSourceOne(src, vm); - if (priv && priv->nbdkitProcess && - qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0) - VIR_WARN("Unable to stop nbdkit for storage source '%s'", qemuBlockStorageSourceGetStorageNodename(src)); + for (backing = src; backing != NULL; backing = backing->backingStore) { + qemuNbdkitStopStorageSourceOne(backing, vm); } } diff --git a/src/qemu/qemu_nbdkit.h b/src/qemu/qemu_nbdkit.h index 853b2cca6f..637bf962a7 100644 --- a/src/qemu/qemu_nbdkit.h +++ b/src/qemu/qemu_nbdkit.h @@ -63,11 +63,13 @@ qemuNbdkitReconnectStorageSource(virStorageSource *source, int qemuNbdkitStartStorageSource(virQEMUDriver *driver, virDomainObj *vm, - virStorageSource *src); + virStorageSource *src, + bool chain); void qemuNbdkitStopStorageSource(virStorageSource *src, - virDomainObj *vm); + virDomainObj *vm, + bool chain); int qemuNbdkitStorageSourceManageProcess(virStorageSource *src, -- 2.43.0
When starting nbdkit processes for the backing store of a disk, we were returning an error if any backing store failed, but we were not cleaning up processes that succeeded higher in the chain. Make sure that if we return a failure status from qemuNbdkitStartStorageSource() that we roll back any processes that had been started. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_nbdkit.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index 39f9c58a48..edbe2137d0 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -920,8 +920,11 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver, return qemuNbdkitStartStorageSourceOne(driver, vm, src); for (backing = src; backing != NULL; backing = backing->backingStore) { - if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) + if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) { + /* roll back any previously-started sources */ + qemuNbdkitStopStorageSource(src, vm, true); return -1; + } } return 0; -- 2.43.0
On Thu, Jan 25, 2024 at 14:52:10 -0600, Jonathon Jongsma wrote:
When starting nbdkit processes for the backing store of a disk, we were returning an error if any backing store failed, but we were not cleaning up processes that succeeded higher in the chain. Make sure that if we return a failure status from qemuNbdkitStartStorageSource() that we roll back any processes that had been started.
qemuNbdkitStartStorageSource is called from: src/qemu/qemu_extdevice.c=qemuExtDevicesStart(virQEMUDriver *driver, src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0) src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram, true) < 0) The counterpart is qemuExtDevicesStop. src/qemu/qemu_hotplug.c=qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, src/qemu/qemu_hotplug.c: if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0) This has cleanup inline. Thus I don't see a possibility where what you describe in the commit message can happen.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_nbdkit.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index 39f9c58a48..edbe2137d0 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -920,8 +920,11 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver, return qemuNbdkitStartStorageSourceOne(driver, vm, src);
for (backing = src; backing != NULL; backing = backing->backingStore) { - if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) + if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) { + /* roll back any previously-started sources */ + qemuNbdkitStopStorageSource(src, vm, true); return -1; + } }
return 0; -- 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On 1/29/24 2:30 AM, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 14:52:10 -0600, Jonathon Jongsma wrote:
When starting nbdkit processes for the backing store of a disk, we were returning an error if any backing store failed, but we were not cleaning up processes that succeeded higher in the chain. Make sure that if we return a failure status from qemuNbdkitStartStorageSource() that we roll back any processes that had been started.
qemuNbdkitStartStorageSource is called from:
src/qemu/qemu_extdevice.c=qemuExtDevicesStart(virQEMUDriver *driver, src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0) src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram, true) < 0)
The counterpart is qemuExtDevicesStop.
src/qemu/qemu_hotplug.c=qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, src/qemu/qemu_hotplug.c: if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
This has cleanup inline.
Thus I don't see a possibility where what you describe in the commit message can happen.
Oops it seems that i never actually responded to this comment. Perhaps because I was waiting for a resolution on patch 3. The motivation for this patch was actually in anticipation of patch 3. In that patch, I followed the existing pattern inside of qemuDomainStorageSourceAccessModify() where we only set a 'revoke_foo' variable if the initial 'allow' call succeeds. In other words, if a function like qemuDomainStorageSourceAccessModifyNVMe() fails, we assume that it has left things in a consistent state and doesn't need to be revoked in the error path. So I followed the same pattern for this function. If you think it would be better, I could just squash this patch with patch 3? Jonathon
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_nbdkit.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index 39f9c58a48..edbe2137d0 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -920,8 +920,11 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver, return qemuNbdkitStartStorageSourceOne(driver, vm, src);
for (backing = src; backing != NULL; backing = backing->backingStore) { - if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) + if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0) { + /* roll back any previously-started sources */ + qemuNbdkitStopStorageSource(src, vm, true); return -1; + } }
return 0; -- 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Feb 08, 2024 at 15:11:25 -0600, Jonathon Jongsma wrote:
On 1/29/24 2:30 AM, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 14:52:10 -0600, Jonathon Jongsma wrote:
When starting nbdkit processes for the backing store of a disk, we were returning an error if any backing store failed, but we were not cleaning up processes that succeeded higher in the chain. Make sure that if we return a failure status from qemuNbdkitStartStorageSource() that we roll back any processes that had been started.
qemuNbdkitStartStorageSource is called from:
src/qemu/qemu_extdevice.c=qemuExtDevicesStart(virQEMUDriver *driver, src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0) src/qemu/qemu_extdevice.c: if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram, true) < 0)
The counterpart is qemuExtDevicesStop.
src/qemu/qemu_hotplug.c=qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, src/qemu/qemu_hotplug.c: if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
This has cleanup inline.
Thus I don't see a possibility where what you describe in the commit message can happen.
Oops it seems that i never actually responded to this comment. Perhaps because I was waiting for a resolution on patch 3.
The motivation for this patch was actually in anticipation of patch 3. In that patch, I followed the existing pattern inside of qemuDomainStorageSourceAccessModify() where we only set a 'revoke_foo' variable if the initial 'allow' call succeeds. In other words, if a function like qemuDomainStorageSourceAccessModifyNVMe() fails, we assume that it has left things in a consistent state and doesn't need to be revoked in the error path. So I followed the same pattern for this function.
If you think it would be better, I could just squash this patch with patch 3?
Fair enough; Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Previously we were only starting or stopping nbdkit when the guest was started or stopped or when hotplugging/unplugging a disk. But when doing block operations, the disk backing store sources can also be be added or removed independently of the disk device. When this happens the nbdkit backend was not being handled properly. For example, when doing a blockcopy from a nbdkit-backed disk to a new disk and pivoting to that new location, the nbdkit process did not get cleaned up properly. Add some functionality to qemuDomainStorageSourceAccessModify() to handle this scenario. Since we're now potentially starting nbdkit from another place in the code, add a check to qemuNbdkitProcessStart() to report an error if we are trying to start nbdkit for a disk source that already has a running nbdkit process. This should never happen. If it does, it indicates an error in another part of our code. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++++++++ src/qemu/qemu_nbdkit.c | 7 +++++++ 2 files changed, 17 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de36641137..973a9af0b6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8026,6 +8026,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, bool revoke_namespace = false; bool revoke_nvme = false; bool revoke_lockspace = false; + bool revoke_nbdkit = false; VIR_DEBUG("src='%s' readonly=%d force_ro=%d force_rw=%d revoke=%d chain=%d", NULLSTR(src->path), src->readonly, force_ro, force_rw, revoke, chain); @@ -8044,6 +8045,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, revoke_namespace = true; revoke_nvme = true; revoke_lockspace = true; + revoke_nbdkit = true; ret = 0; goto revoke; } @@ -8059,6 +8061,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, revoke_nvme = true; + if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0) + goto revoke; + + revoke_nbdkit = true; + if (qemuDomainNamespaceSetupDisk(vm, src, &revoke_namespace) < 0) goto revoke; } @@ -8113,6 +8120,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, VIR_WARN("Unable to release lock on %s", srcstr); } + if (revoke_nbdkit) + qemuNbdkitStopStorageSource(src, vm, chain); + cleanup: src->readonly = was_readonly; virErrorRestore(&orig_err); diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index edbe2137d0..73dc90af3e 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc, struct nbd_handle *nbd = NULL; #endif + /* don't try to start nbdkit again if we've already started it */ + if (proc->pid > 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempting to start nbdkit twice")); + return -1; + } + if (!(cmd = qemuNbdkitProcessBuildCommand(proc))) return -1; -- 2.43.0
On Thu, Jan 25, 2024 at 14:52:11 -0600, Jonathon Jongsma wrote:
Previously we were only starting or stopping nbdkit when the guest was started or stopped or when hotplugging/unplugging a disk. But when doing block operations, the disk backing store sources can also be be added or removed independently of the disk device. When this happens the nbdkit backend was not being handled properly. For example, when doing a blockcopy from a nbdkit-backed disk to a new disk and pivoting to that new location, the nbdkit process did not get cleaned up properly. Add some functionality to qemuDomainStorageSourceAccessModify() to handle this scenario.
Since we're now potentially starting nbdkit from another place in the code, add a check to qemuNbdkitProcessStart() to report an error if we are trying to start nbdkit for a disk source that already has a running nbdkit process. This should never happen. If it does, it indicates an error in another part of our code.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++++++++ src/qemu/qemu_nbdkit.c | 7 +++++++ 2 files changed, 17 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de36641137..973a9af0b6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
@@ -8059,6 +8061,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
revoke_nvme = true;
+ if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0) + goto revoke; + + revoke_nbdkit = true; +
Note that qemuDomainStorageSourceAccessModify can be called on already existing 'virStorageSource' to e.g. change from read-only to read-write mode or back. [...]
if (qemuDomainNamespaceSetupDisk(vm, src, &revoke_namespace) < 0) goto revoke; }
@@ -8113,6 +8120,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, VIR_WARN("Unable to release lock on %s", srcstr); }
+ if (revoke_nbdkit) + qemuNbdkitStopStorageSource(src, vm, chain); + cleanup: src->readonly = was_readonly; virErrorRestore(&orig_err); diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index edbe2137d0..73dc90af3e 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc, struct nbd_handle *nbd = NULL; #endif
+ /* don't try to start nbdkit again if we've already started it */ + if (proc->pid > 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempting to start nbdkit twice")); + return -1; + }
[...] Which means that this WILL get hit multiple times. Additionally don't forget that qemuDomainStorageSourceAccessModify is called from. qemuDomainStorageSourceChainAccessAllow. Which is e.g. called from qemuDomainAttachDeviceDiskLiveInternal where we have this code: if (!virStorageSourceIsEmpty(disk->src)) { if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup; if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) goto cleanup; if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) ^^^^^^^^^^^^^^^^^^^ goto cleanup; releaseSeclabel = true; if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0) goto cleanup; if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0) goto cleanup; if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0) ^^^^^^^^^^^^^^^^^ goto cleanup; }
+ if (!(cmd = qemuNbdkitProcessBuildCommand(proc))) return -1;
Also conceptually the qemuDomainStorageSourceAccessModify function deals with security and permissions of image files. I don't think that the NBDKit helper process code belongs into it. NACK
On 1/29/24 2:25 AM, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 14:52:11 -0600, Jonathon Jongsma wrote:
Previously we were only starting or stopping nbdkit when the guest was started or stopped or when hotplugging/unplugging a disk. But when doing block operations, the disk backing store sources can also be be added or removed independently of the disk device. When this happens the nbdkit backend was not being handled properly. For example, when doing a blockcopy from a nbdkit-backed disk to a new disk and pivoting to that new location, the nbdkit process did not get cleaned up properly. Add some functionality to qemuDomainStorageSourceAccessModify() to handle this scenario.
Since we're now potentially starting nbdkit from another place in the code, add a check to qemuNbdkitProcessStart() to report an error if we are trying to start nbdkit for a disk source that already has a running nbdkit process. This should never happen. If it does, it indicates an error in another part of our code.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++++++++ src/qemu/qemu_nbdkit.c | 7 +++++++ 2 files changed, 17 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de36641137..973a9af0b6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
@@ -8059,6 +8061,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver,
revoke_nvme = true;
+ if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0) + goto revoke; + + revoke_nbdkit = true; +
Note that qemuDomainStorageSourceAccessModify can be called on already existing 'virStorageSource' to e.g. change from read-only to read-write mode or back.
That's precisely why I put it inside of this block: if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) { And it seems that the MODIFY_ACCESS flag is always set unless we are adding a new source since this is also the way that new nvme devices are set up.
[...]
if (qemuDomainNamespaceSetupDisk(vm, src, &revoke_namespace) < 0) goto revoke; }
@@ -8113,6 +8120,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, VIR_WARN("Unable to release lock on %s", srcstr); }
+ if (revoke_nbdkit) + qemuNbdkitStopStorageSource(src, vm, chain); + cleanup: src->readonly = was_readonly; virErrorRestore(&orig_err); diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index edbe2137d0..73dc90af3e 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc, struct nbd_handle *nbd = NULL; #endif
+ /* don't try to start nbdkit again if we've already started it */ + if (proc->pid > 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempting to start nbdkit twice")); + return -1; + }
[...] Which means that this WILL get hit multiple times. Additionally don't forget that qemuDomainStorageSourceAccessModify is called from. qemuDomainStorageSourceChainAccessAllow.
Which is e.g. called from qemuDomainAttachDeviceDiskLiveInternal where we have this code:
if (!virStorageSourceIsEmpty(disk->src)) { if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup;
if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) goto cleanup;
if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0)
^^^^^^^^^^^^^^^^^^^
goto cleanup;
releaseSeclabel = true;
if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0) goto cleanup;
if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0) goto cleanup;
if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
^^^^^^^^^^^^^^^^^
goto cleanup; }
As you point out, I didn't catch this case. It seems to me that the right approach here is to just remove the call to qemuNbdkitStartStorageSource() from here and instead rely on the qemuDomainStorageSourceChainAccessAllow() call above to initialize nbdkit.
+ if (!(cmd = qemuNbdkitProcessBuildCommand(proc))) return -1;
Also conceptually the qemuDomainStorageSourceAccessModify function deals with security and permissions of image files. I don't think that the NBDKit helper process code belongs into it.
NACK
Fair point, but we'll still need to do something to start the nbdkit process in the same basic code path as these calls to AccessAllow()/AccessRevoke() if we want to support nbdkit-backed disks in these paths. And IMO it's not all that dissimilar to the work that we're doing here to detach the nvme devices from the host, etc. Or is there a better spot to handle this? Jonathon
On Mon, Jan 29, 2024 at 16:38:44 -0600, Jonathon Jongsma wrote:
On 1/29/24 2:25 AM, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 14:52:11 -0600, Jonathon Jongsma wrote:
[...]
[...]
@@ -8059,6 +8061,11 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, revoke_nvme = true; + if (qemuNbdkitStartStorageSource(driver, vm, src, chain) < 0) + goto revoke; + + revoke_nbdkit = true; +
Note that qemuDomainStorageSourceAccessModify can be called on already existing 'virStorageSource' to e.g. change from read-only to read-write mode or back.
That's precisely why I put it inside of this block:
if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) {
And it seems that the MODIFY_ACCESS flag is always set unless we are adding a new source since this is also the way that new nvme devices are set up.
[...]
if (qemuDomainNamespaceSetupDisk(vm, src, &revoke_namespace) < 0) goto revoke; }
@@ -8113,6 +8120,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriver *driver, VIR_WARN("Unable to release lock on %s", srcstr); } + if (revoke_nbdkit) + qemuNbdkitStopStorageSource(src, vm, chain); + cleanup: src->readonly = was_readonly; virErrorRestore(&orig_err); diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index edbe2137d0..73dc90af3e 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -1195,6 +1195,13 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc, struct nbd_handle *nbd = NULL; #endif + /* don't try to start nbdkit again if we've already started it */ + if (proc->pid > 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempting to start nbdkit twice")); + return -1; + }
[...] Which means that this WILL get hit multiple times. Additionally don't forget that qemuDomainStorageSourceAccessModify is called from. qemuDomainStorageSourceChainAccessAllow.
Which is e.g. called from qemuDomainAttachDeviceDiskLiveInternal where we have this code:
if (!virStorageSourceIsEmpty(disk->src)) { if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup;
if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) goto cleanup;
if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0)
^^^^^^^^^^^^^^^^^^^
goto cleanup;
releaseSeclabel = true;
if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0) goto cleanup;
if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0) goto cleanup;
if (qemuNbdkitStartStorageSource(driver, vm, disk->src, true) < 0)
^^^^^^^^^^^^^^^^^
goto cleanup; }
As you point out, I didn't catch this case. It seems to me that the right approach here is to just remove the call to qemuNbdkitStartStorageSource() from here and instead rely on the qemuDomainStorageSourceChainAccessAllow() call above to initialize nbdkit.
+ if (!(cmd = qemuNbdkitProcessBuildCommand(proc))) return -1;
Also conceptually the qemuDomainStorageSourceAccessModify function deals with security and permissions of image files. I don't think that the NBDKit helper process code belongs into it.
NACK
Fair point, but we'll still need to do something to start the nbdkit process in the same basic code path as these calls to AccessAllow()/AccessRevoke() if we want to support nbdkit-backed disks in these paths. And IMO it's not all that dissimilar to the work that we're doing here to detach the nvme devices from the host, etc. Or is there a better spot to handle this?
Hmm, good point. I didn't realize that the setup of the NVMe device is so complex and that it has crept into this function. Based on that I can't really argue against setting up nbdkit there too. Once you fix the issue I've pointed out above: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Thu, Jan 25, 2024 at 14:52:09 -0600, Jonathon Jongsma wrote:
This will allow us to start or stop nbdkit for just a single disk source or for every source in the backing chain. This will be used in following patches.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_extdevice.c | 8 +++--- src/qemu/qemu_hotplug.c | 6 ++--- src/qemu/qemu_nbdkit.c | 51 ++++++++++++++++++++++++++++++--------- src/qemu/qemu_nbdkit.h | 6 +++-- 4 files changed, 51 insertions(+), 20 deletions(-)
[...]
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index 1c72b6fe6a..39f9c58a48 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -912,18 +928,31 @@ qemuNbdkitStartStorageSource(virQEMUDriver *driver, }
+static void +qemuNbdkitStopStorageSourceOne(virStorageSource *src, + virDomainObj *vm) +{ + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + + if (priv && priv->nbdkitProcess && + qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0) + VIR_WARN("Unable to stop nbdkit for storage source '%s'", + qemuBlockStorageSourceGetStorageNodename(src)); +} + + void
No return value.
qemuNbdkitStopStorageSource(virStorageSource *src, - virDomainObj *vm) + virDomainObj *vm, + bool chain) { virStorageSource *backing;
- for (backing = src; backing != NULL; backing = backing->backingStore) { - qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(backing); + if (! chain)
s/! chain/!chain/
+ return qemuNbdkitStopStorageSourceOne(src, vm);
Return in a function with no return value. Compilers don't complain but it may mislead readers.
- if (priv && priv->nbdkitProcess && - qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0) - VIR_WARN("Unable to stop nbdkit for storage source '%s'", qemuBlockStorageSourceGetStorageNodename(src)); + for (backing = src; backing != NULL; backing = backing->backingStore) { + qemuNbdkitStopStorageSourceOne(backing, vm);
Alternatively rather than having an extra invocation before this loop you can break out if (!chain) here for same result. Same applies also to the 'Start' function.
} }
With the extra space and 'return' removed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Jonathon Jongsma -
Peter Krempa