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