Re: [PATCH 2/3] qemu: roll back if not all nbdkit backends are successful
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(a)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(a)lists.libvirt.org
> To unsubscribe send an email to devel-leave(a)lists.libvirt.org