[libvirt] [PATCH 0/4] qemu: blockdev-related cleanups and refactors (blockdev-add saga)
Peter Krempa (4): qemu: blockjob: Don't reset state when entering sync blockjob qemu: blockjob: Don't emit traditional disk events for jobs without disk qemu: Refactor variables for extracting flags in qemuDomainBlockCopyCommon qemu: block: Split up qemuBlockStorageSourceAttachApply src/qemu/qemu_block.c | 86 +++++++++++++++++++++++++++++----------- src/qemu/qemu_blockjob.c | 5 ++- src/qemu/qemu_driver.c | 20 +++++----- 3 files changed, 77 insertions(+), 34 deletions(-) -- 2.21.0
job->newstate is now used internally all the time so there's no need to clear it as it already has correct value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 34a4047210..a4662342a7 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -428,7 +428,6 @@ qemuBlockJobSyncBegin(qemuBlockJobDataPtr job) VIR_DEBUG("disk=%s", NULLSTR(diskdst)); job->synchronous = true; - job->newstate = -1; } -- 2.21.0
With -blockdev it will be possible that a block job loses the disk that was used to start it to a guest-initiated hot-unplug. Don't emit the block job events in that case as we can't report the top level source or disk target for an unplugged (and potentially replugged with different source) disk. Eventually when we add machinery for tracking jobs globally for a VM the event will be reinstated via the domain job event. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a4662342a7..b3bdbeb990 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -216,6 +216,10 @@ qemuBlockJobEmitEvents(virQEMUDriverPtr driver, virObjectEventPtr event = NULL; virObjectEventPtr event2 = NULL; + /* don't emit events for jobs without disk */ + if (!disk) + return; + /* don't emit events for internal jobs and states */ if (type >= VIR_DOMAIN_BLOCK_JOB_TYPE_LAST || status >= VIR_DOMAIN_BLOCK_JOB_LAST) -- 2.21.0
Add separate booleans for extracting VIR_DOMAIN_BLOCK_COPY_REUSE_EXT and VIR_DOMAIN_BLOCK_COPY_SHALLOW from '@flags' and also change 'reuse' into 'existing'. qemuMonitorDriveMirror requires the unmodified state of the flags to pass to qemu and also we use the value a few times internally. Extract it separately now. The 'reuse' flag did not indicate reusing of the file as much as the fact that the storage is existing and thus should not be created, so modify the name to reflect this. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 140896f329..1955e871ec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17507,7 +17507,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, bool need_unlink = false; VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; - bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); + bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); + bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW); + bool existing = mirror_reuse; qemuBlockJobDataPtr job = NULL; VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); @@ -17571,8 +17573,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* unless the user provides a pre-created file, shallow copy into a raw * file is not possible */ - if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && !reuse && - mirror->format == VIR_STORAGE_FILE_RAW) { + if (mirror_shallow && !existing && mirror->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("shallow copy of disk '%s' into a raw file " "is not possible"), @@ -17591,11 +17592,11 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainStorageFileInit(driver, vm, mirror, NULL) < 0) goto endjob; - if (qemuDomainBlockCopyValidateMirror(mirror, disk->dst, &reuse) < 0) + if (qemuDomainBlockCopyValidateMirror(mirror, disk->dst, &existing) < 0) goto endjob; if (!mirror->format) { - if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { + if (!mirror_reuse) { mirror->format = disk->src->format; } else { /* If the user passed the REUSE_EXT flag, then either they @@ -17618,7 +17619,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, } /* pre-create the image file */ - if (!reuse) { + if (!existing) { if (virStorageFileCreate(mirror) < 0) { virReportSystemError(errno, "%s", _("failed to create copy target")); goto endjob; @@ -17636,7 +17637,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* If reusing an external image that includes a backing file but the user * did not enumerate the chain in the XML we need to detect the chain */ - if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && + if (mirror_reuse && mirror->format >= VIR_STORAGE_FILE_BACKING && mirror->backingStore == NULL && qemuDomainDetermineDiskChain(driver, vm, disk, mirror, true) < 0) @@ -17653,11 +17654,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); /* qemuMonitorDriveMirror needs to honor the REUSE_EXT flag as specified - * by the user regardless of how @reuse was modified */ + * by the user */ ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format, bandwidth, granularity, buf_size, - flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW, - flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); + mirror_shallow, mirror_reuse); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; -- 2.21.0
Split up the addition of a storage source into the following sub-steps: 1) storage access dependancies (TLS transport, persistent reservation) 2) storage acccess node (file/gluster/nbd...) 3) format driver dependancies (encryption secret) 4) format driver node (qcow2, raw, ...) The functions split out will be later reused when implementing support for 'blockdev-create' as we'll need the dependancies plugged in first, then blockdev-create will be called and after that successfully finishes blockdev-add will be added. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 86 +++++++++++++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 0a6522577d..8641e2011c 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1450,25 +1450,10 @@ qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSourcePtr src) } -/** - * qemuBlockStorageSourceAttachApply: - * @mon: monitor object - * @data: structure holding data of block device to apply - * - * Attaches a virStorageSource definition converted to - * qemuBlockStorageSourceAttachData to a running VM. This function expects being - * called after the monitor was entered. - * - * Returns 0 on success and -1 on error with a libvirt error reported. If an - * error occurred, changes which were already applied need to be rolled back by - * calling qemuBlockStorageSourceAttachRollback. - */ -int -qemuBlockStorageSourceAttachApply(qemuMonitorPtr mon, - qemuBlockStorageSourceAttachDataPtr data) +static int +qemuBlockStorageSourceAttachApplyStorageDeps(qemuMonitorPtr mon, + qemuBlockStorageSourceAttachDataPtr data) { - int rv; - if (data->prmgrProps && qemuMonitorAddObject(mon, &data->prmgrProps, &data->prmgrAlias) < 0) return -1; @@ -1478,15 +1463,20 @@ qemuBlockStorageSourceAttachApply(qemuMonitorPtr mon, &data->authsecretAlias) < 0) return -1; - if (data->encryptsecretProps && - qemuMonitorAddObject(mon, &data->encryptsecretProps, - &data->encryptsecretAlias) < 0) - return -1; - if (data->tlsProps && qemuMonitorAddObject(mon, &data->tlsProps, &data->tlsAlias) < 0) return -1; + return 0; +} + + +static int +qemuBlockStorageSourceAttachApplyStorage(qemuMonitorPtr mon, + qemuBlockStorageSourceAttachDataPtr data) +{ + int rv; + if (data->storageProps) { rv = qemuMonitorBlockdevAdd(mon, data->storageProps); data->storageProps = NULL; @@ -1497,6 +1487,29 @@ qemuBlockStorageSourceAttachApply(qemuMonitorPtr mon, data->storageAttached = true; } + return 0; +} + + +static int +qemuBlockStorageSourceAttachApplyFormatDeps(qemuMonitorPtr mon, + qemuBlockStorageSourceAttachDataPtr data) +{ + if (data->encryptsecretProps && + qemuMonitorAddObject(mon, &data->encryptsecretProps, + &data->encryptsecretAlias) < 0) + return -1; + + return 0; +} + + +static int +qemuBlockStorageSourceAttachApplyFormat(qemuMonitorPtr mon, + qemuBlockStorageSourceAttachDataPtr data) +{ + int rv; + if (data->formatProps) { rv = qemuMonitorBlockdevAdd(mon, data->formatProps); data->formatProps = NULL; @@ -1507,6 +1520,33 @@ qemuBlockStorageSourceAttachApply(qemuMonitorPtr mon, data->formatAttached = true; } + return 0; +} + + +/** + * qemuBlockStorageSourceAttachApply: + * @mon: monitor object + * @data: structure holding data of block device to apply + * + * Attaches a virStorageSource definition converted to + * qemuBlockStorageSourceAttachData to a running VM. This function expects being + * called after the monitor was entered. + * + * Returns 0 on success and -1 on error with a libvirt error reported. If an + * error occurred, changes which were already applied need to be rolled back by + * calling qemuBlockStorageSourceAttachRollback. + */ +int +qemuBlockStorageSourceAttachApply(qemuMonitorPtr mon, + qemuBlockStorageSourceAttachDataPtr data) +{ + if (qemuBlockStorageSourceAttachApplyStorageDeps(mon, data) < 0 || + qemuBlockStorageSourceAttachApplyStorage(mon, data) < 0 || + qemuBlockStorageSourceAttachApplyFormatDeps(mon, data) < 0 || + qemuBlockStorageSourceAttachApplyFormat(mon, data) < 0) + return -1; + if (data->driveCmd) { if (qemuMonitorAddDrive(mon, data->driveCmd) < 0) return -1; -- 2.21.0
On Thu, Jul 11, 2019 at 04:56:05PM +0200, Peter Krempa wrote:
Split up the addition of a storage source into the following sub-steps: 1) storage access dependancies (TLS transport, persistent reservation)
dependencies
2) storage acccess node (file/gluster/nbd...) 3) format driver dependancies (encryption secret)
dependencies
4) format driver node (qcow2, raw, ...)
The functions split out will be later reused when implementing support for 'blockdev-create' as we'll need the dependancies plugged in first,
dependencies
then blockdev-create will be called and after that successfully finishes blockdev-add will be added.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 86 +++++++++++++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 23 deletions(-)
Jano
On Thu, Jul 11, 2019 at 04:56:01PM +0200, Peter Krempa wrote:
Peter Krempa (4): qemu: blockjob: Don't reset state when entering sync blockjob qemu: blockjob: Don't emit traditional disk events for jobs without disk qemu: Refactor variables for extracting flags in qemuDomainBlockCopyCommon qemu: block: Split up qemuBlockStorageSourceAttachApply
src/qemu/qemu_block.c | 86 +++++++++++++++++++++++++++++----------- src/qemu/qemu_blockjob.c | 5 ++- src/qemu/qemu_driver.c | 20 +++++----- 3 files changed, 77 insertions(+), 34 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa