----- Original Message ----- | From: "Peter Krempa" <pkre...@redhat.com> | To: "Shanzhi Yu" <s...@redhat.com> | Cc: libvir-list@redhat.com | Sent: Wednesday, March 25, 2015 11:24:21 PM | Subject: Re: [libvirt] [PATCH] conf: refact virDomainHasDiskMirror and rename | it to virDomainHasBlockjob
| On Tue, Mar 24, 2015 at 18:08:00 +0800, Shanzhi Yu wrote: | > Create external snapshot or migrate a vm when there is a blockpull | > job running should be forbidden by libvirt, otherwise qemu try to | > create external snapshot and failed with error "unable to execute | > QEMU command 'transaction': Device 'drive-virtio-disk0' is busy: | > block device is in use by block job: stream", and migration will | > succeed which will lead the blockpull job failed. | > | > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1203628 | > Signed-off-by: Shanzhi Yu <s...@redhat.com> | > --- | > src/conf/domain_conf.c | 6 +++--- | > src/conf/domain_conf.h | 2 +- | > src/libvirt_private.syms | 2 +- | > src/qemu/qemu_driver.c | 6 +++--- | > src/qemu/qemu_migration.c | 2 +- | > 5 files changed, 9 insertions(+), 9 deletions(-) | > | > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c | > index d633f93..24445af 100644 | > --- a/src/conf/domain_conf.c | > +++ b/src/conf/domain_conf.c | > @@ -12264,13 +12264,13 @@ virDomainDiskRemoveByName(virDomainDefPtr def, | > const char *name) | > } | > | > /* Return true if VM has at least one disk involved in a current block | > - * copy/commit job (that is, with a <mirror> element in the disk xml). */ | > + * copy/commit/pull job */ | > bool | > -virDomainHasDiskMirror(virDomainObjPtr vm) | > +virDomainHasBlockjob(virDomainObjPtr vm) | > { | > size_t i; | > for (i = 0; i < vm->def->ndisks; i++) | > - if (vm->def->disks[i]->mirror) | > + if (vm->def->disks[i]->blockjob) | > return true; | > return false; | > } | > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h | > index bceb2d7..32674f3 100644 | > --- a/src/conf/domain_conf.h | > +++ b/src/conf/domain_conf.h | > @@ -2645,7 +2645,7 @@ int virDomainDiskSourceParse(xmlNodePtr node, | > xmlXPathContextPtr ctxt, | > virStorageSourcePtr src); | > | > -bool virDomainHasDiskMirror(virDomainObjPtr vm); | > +bool virDomainHasBlockjob(virDomainObjPtr vm); | > | > int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net); | > virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char | > *device); | > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms | > index 33222f0..9ebaf4a 100644 | > --- a/src/libvirt_private.syms | > +++ b/src/libvirt_private.syms | > @@ -297,7 +297,7 @@ virDomainGraphicsTypeFromString; | > virDomainGraphicsTypeToString; | > virDomainGraphicsVNCSharePolicyTypeFromString; | > virDomainGraphicsVNCSharePolicyTypeToString; | > -virDomainHasDiskMirror; | > +virDomainHasBlockjob; | > virDomainHasNet; | > virDomainHostdevCapsTypeToString; | > virDomainHostdevDefAlloc; | > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c | > index 80a3d77..51e302f 100644 | > --- a/src/qemu/qemu_driver.c | > +++ b/src/qemu/qemu_driver.c | > @@ -7398,7 +7398,7 @@ static virDomainPtr | > qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml | > | > virObjectRef(vm); | > def = NULL; | > - if (virDomainHasDiskMirror(vm)) { | > + if (virDomainHasBlockjob(vm)) { | > virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", | > _("domain has active block job")); | > virDomainObjAssignDef(vm, NULL, false, NULL); | This code here has two effects: | 1) if the VM is started, this is to forbid defining it back | 2) if the VM is not started it forbids defining one with a disk mirror | element | Additionally this check (in the existing form) is wrong since the active | block copy job should not interlock domain definition. | Whith this patch the check would be even more wrong as it would disallow | changes to the persistent defintion if any block job was active. Understood | This problem needs to be fixed separately, before attempting such | change. | > @@ -14583,7 +14583,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, | > "%s", _("domain is marked for auto destroy")); | > goto cleanup; | > } | > - if (virDomainHasDiskMirror(vm)) { | > + if (virDomainHasBlockjob(vm)) { | > virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", | > _("domain has active block job")); | > goto cleanup; | I think we should reconsider whether we want to block snapshots by a | single copy job or we want to do the check in a more granular (per-disk) Yes, it's better check per disk. | fashion. | > @@ -15245,7 +15245,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr | > snapshot, | > if (!(caps = virQEMUDriverGetCapabilities(driver, false))) | > goto cleanup; | > | > - if (virDomainHasDiskMirror(vm)) { | > + if (virDomainHasBlockjob(vm)) { | > virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", | > _("domain has active block job")); | > goto cleanup; | Here it's questionable whether combining the check for | > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c | > index d34bb02..27a76ec 100644 | > --- a/src/qemu/qemu_migration.c | > +++ b/src/qemu/qemu_migration.c | > @@ -1977,7 +1977,7 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, | > virDomainObjPtr vm, | > | > } | > | > - if (virDomainHasDiskMirror(vm)) { | > + if (virDomainHasBlockjob(vm)) { | > virReportError(VIR_ERR_OPERATION_INVALID, "%s", | > _("domain has an active block job")); | > return false; | Here it should be okay. | > -- | I'll take this patch over and use it along when fixing the rest of the | problems I've pointed out here. OK, thanks | Peter -- Regards shyu
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list