Re: [libvirt] [PATCH v2 04/22] qemu: Use domain condition for synchronous block jobs

On 06/02/2015 08:34 AM, Jiri Denemark wrote: > By switching block jobs to use domain conditions, we can drop some > pretty complicated code in NBD storage migration. Moreover, we are > getting ready for migration events (to replace our 50ms polling on > query-migrate). The ultimate goal is to have a single loop waiting > (virDomainObjWait) for any migration related event (changed status of a > migration, disk mirror events, internal abort requests, ...). This patch > makes NBD storage migration ready for this: first we call a QMP command > to start or cancel drive mirror on all disks we are interested in and > then we wait for a single condition which is signaled on any event > related to any of the mirrors. > > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; > --- > > Notes: > Version 2: > - slightly modified to use domain conditions > > po/POTFILES.in | 1 - > src/qemu/qemu_blockjob.c | 137 ++------------------- > src/qemu/qemu_blockjob.h | 12 +- > src/qemu/qemu_domain.c | 17 +-- > src/qemu/qemu_domain.h | 1 - > src/qemu/qemu_driver.c | 24 ++-- > src/qemu/qemu_migration.c | 299 ++++++++++++++++++++++++++-------------------- > src/qemu/qemu_process.c | 13 +- > 8 files changed, 197 insertions(+), 307 deletions(-) > ... > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 93e29e7..61b3e34 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -1720,7 +1720,7 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver, > > > /** > - * qemuMigrationCheckDriveMirror: > + * qemuMigrationDriveMirrorReady: > * @driver: qemu driver > * @vm: domain > * > @@ -1733,111 +1733,148 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver, > * -1 on error. > */ > static int > -qemuMigrationCheckDriveMirror(virQEMUDriverPtr driver, > +qemuMigrationDriveMirrorReady(virQEMUDriverPtr driver, > virDomainObjPtr vm) > { > size_t i; > - int ret = 1; > + size_t notReady = 0; > + int status; > > for (i = 0; i < vm->def->ndisks; i++) { > virDomainDiskDefPtr disk = vm->def->disks[i]; > qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > > - if (!diskPriv->migrating || !diskPriv->blockJobSync) > + if (!diskPriv->migrating) > continue; > > - /* process any pending event */ > - if (qemuBlockJobSyncWaitWithTimeout(driver, vm, disk, > - 0ull, NULL) < 0) > - return -1; > - > - switch (disk->mirrorState) { > - case VIR_DOMAIN_DISK_MIRROR_STATE_NONE: > - ret = 0; > - break; > - case VIR_DOMAIN_DISK_MIRROR_STATE_ABORT: > + status = qemuBlockJobUpdate(driver, vm, disk); > + if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { ah - and now I see it cannot be a void function; however, this could cause Coverity checker to complain about other uses. I wasn't able to apply the series to my Coverity branch since it wouldn't git am apply. Still that leaves me wondering why other paths don't check the status. They may not care, but with one or more paths checking (as seen in the rest of this module) it leaves a future reader not knowing whether the paths that aren't checking should check.