<no commit message>?
On 01/07/2015 10:42 AM, Ján Tomko wrote:
> ---
> src/qemu/qemu_driver.c | 131 +++++++++++++++++++++++++++++-----------------
> src/qemu/qemu_migration.c | 98 +++++++++++++++-------------------
> 2 files changed, 125 insertions(+), 104 deletions(-)
>
Should qemu_migration move to 10/14?
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1781ea9..55d6fb3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1978,7 +1978,8 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSystemPowerdown(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
> }
>
> endjob:
> @@ -2073,7 +2074,8 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
> } else {
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSystemPowerdown(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> if (ret == 0)
> qemuDomainSetFakeReboot(driver, vm, isReboot);
> @@ -2116,7 +2118,8 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags)
> priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSystemReset(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> priv->fakeReboot = false;
>
> @@ -2333,7 +2336,8 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
> priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> r = qemuMonitorSetBalloon(priv->mon, newmem);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
Here we have another Audit issue...
> virDomainAuditMemory(vm, vm->def->mem.cur_balloon, newmem, "update",
> r == 1);
> if (r < 0)
> @@ -2415,7 +2419,8 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
>
> qemuDomainObjEnterMonitor(driver, vm);
> r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
> if (r < 0) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("unable to set balloon driver collection period"));
> @@ -2479,7 +2484,8 @@ static int qemuDomainInjectNMI(virDomainPtr domain, unsigned int flags)
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorInjectNMI(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
if ret == 0, then @ cleanup we return success
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
Hmmm... do we need to call DomainObjEndJob if ExitMonitor fails?
I don't even remember if this was an issue for previous such exits...
> @@ -2541,7 +2547,8 @@ static int qemuDomainSendKey(virDomainPtr domain,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
again, if ret == 0, then we return wrong status possibly... Also I see
another EndJob below...
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -2596,7 +2603,10 @@ static int qemuDomainGetInfo(virDomainPtr dom,
> } else {
> qemuDomainObjEnterMonitor(driver, vm);
> err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> + qemuDomainObjEndJob(driver, vm);
Well, I think I have my answer. You may want to go back through all the
changes to see if any are missed...
> + goto cleanup;
> + }
> }
> qemuDomainObjEndJob(driver, vm);
>
> @@ -3492,7 +3502,7 @@ static int qemuDumpToFd(virQEMUDriverPtr driver, virDomainObjPtr vm,
> ret = qemuMonitorDumpToFd(priv->mon, fd, dumpformat);
>
> cleanup:
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
>
> return ret;
> }
> @@ -3691,7 +3701,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
> priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSystemReset(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> }
>
> if (resume && virDomainObjIsActive(vm)) {
> @@ -3788,10 +3798,11 @@ qemuDomainScreenshot(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> if (qemuMonitorScreendump(priv->mon, tmp) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto endjob;
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
>
> if (VIR_CLOSE(tmp_fd) < 0) {
> virReportSystemError(errno, _("unable to close %s"), tmp);
> @@ -4216,7 +4227,8 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &guestFilter);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
Even though it's a void function, I think instead of goto endjob, it
should be ret = -1...
> if (ret < 0)
> goto endjob;
>
> @@ -6227,7 +6239,8 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + err = -1;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -10083,10 +10096,11 @@ qemuDomainBlockResize(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> if (qemuMonitorBlockResize(priv->mon, device, size) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto endjob;
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
>
> ret = 0;
>
> @@ -10740,7 +10754,8 @@ qemuDomainMemoryStats(virDomainPtr dom,
> qemuDomainObjPrivatePtr priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> if (ret >= 0 && ret < nr_stats) {
> long rss;
> @@ -10874,16 +10889,17 @@ qemuDomainMemoryPeek(virDomainPtr dom,
> qemuDomainObjEnterMonitor(driver, vm);
> if (flags == VIR_MEMORY_VIRTUAL) {
> if (qemuMonitorSaveVirtualMemory(priv->mon, offset, size, tmp) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto endjob;
> }
> } else {
> if (qemuMonitorSavePhysicalMemory(priv->mon, offset, size, tmp) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto endjob;
> }
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
>
> /* Read the memory file into buffer. */
> if (saferead(fd, buffer, size) == (ssize_t) -1) {
> @@ -11121,7 +11137,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
> ret = qemuMonitorGetBlockExtent(priv->mon,
> disk->info.alias,
> &src->allocation);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
> }
>
> if (ret == 0) {
> @@ -12361,7 +12378,8 @@ static int qemuDomainAbortJob(virDomainPtr dom)
> qemuDomainObjAbortAsyncJob(vm);
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorMigrateCancel(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -12404,7 +12422,8 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom,
> VIR_DEBUG("Setting migration downtime to %llums", downtime);
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSetMigrationDowntime(priv->mon, downtime);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -12457,7 +12476,8 @@ qemuDomainMigrateGetCompressionCache(virDomainPtr dom,
> ret = qemuMonitorGetMigrationCacheSize(priv->mon, cacheSize);
> }
>
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -12511,7 +12531,8 @@ qemuDomainMigrateSetCompressionCache(virDomainPtr dom,
> ret = qemuMonitorSetMigrationCacheSize(priv->mon, cacheSize);
> }
>
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -12561,7 +12582,8 @@ qemuDomainMigrateSetMaxSpeed(virDomainPtr dom,
> VIR_DEBUG("Setting migration bandwidth to %luMbs", bandwidth);
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSetMigrationSpeed(priv->mon, bandwidth);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> if (ret == 0)
> priv->migMaxBandwidth = bandwidth;
> @@ -15006,7 +15028,10 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorArbitraryCommand(priv->mon, cmd, result, hmp);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> + ret = -1;
> + goto endjob;
This goto seems superfluous... especially compared to other recent
changes. Although seemingly correct, it's handled differently and it's
so much easier when things are more consistent...
Of course each of them could just have ret = qemuDomainobjExitMonitor()
- or at least these inline cases where endjob: is almost always the next
line.
> + }
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -15328,14 +15353,10 @@ qemuDomainBlockPivot(virConnectPtr conn,
> if (!disk->mirrorState) {
> qemuDomainObjEnterMonitor(driver, vm);
> rc = qemuMonitorBlockJobInfo(priv->mon, device, &info, NULL);
> - qemuDomainObjExitMonitor(driver, vm);
> - if (rc < 0)
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("domain is not running"));
> + if (rc < 0)
> goto cleanup;
> - }
> if (rc == 1 && info.cur == info.end &&
> info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY)
> disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> @@ -15412,7 +15433,8 @@ qemuDomainBlockPivot(virConnectPtr conn,
> disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path, format);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
In this case, if ret == 0, then we return success incorrectly. Perhaps
we should set ret = qemuDomainObjExitMonitor here since the next if
handles < 0, although after that there's a SaveStatus, which I wonder if
we really want to do in the case where ret < 0.
>
> if (ret < 0) {
> /* On failure, qemu abandons the mirror, and reverts back to
> @@ -15615,8 +15637,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
> speed, mode, async);
> - qemuDomainObjExitMonitor(driver, vm);
> - if (ret < 0) {
> + if (qemuDomainObjExitMonitor(driver, vm) < 0 ||
> + ret < 0) {
Again - if ret == 0, then what?
FWIW: this is a construct I was going to suggest for many of the other
failure checks where both would fall into an error path, until I started
thinking about the ret == 0 and then Exit == failure case...
> if (mode == BLOCK_JOB_ABORT && disk->mirror)
> disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> goto endjob;
> @@ -15660,7 +15682,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorBlockJobInfo(priv->mon, device, &dummy, NULL);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + break;
Here again, ret == 0 and we're breaking - although the next check makes
me wonder... Furthermore about 10 lines or so dow there's a
virDomainObjIsActive check - is that now not necessary like other places?
>
> if (ret <= 0)
> break;
> @@ -15764,7 +15787,8 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorBlockJobInfo(priv->mon, device, info, &bandwidth);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
Another ret == 0 possibility
> if (ret < 0)
> goto endjob;
>
> @@ -15982,8 +16006,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
> ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format,
> bandwidth, granularity, buf_size, flags);
> virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
OK - here we Audit right after the DriveMirror, then exit the Monitor...
> - qemuDomainObjExitMonitor(driver, vm);
> - if (ret < 0) {
> + if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) {
Another place where ret == 0 could be true...
> qemuDomainPrepareDiskChainElement(driver, vm, mirror,
> VIR_DISK_CHAIN_NO_ACCESS);
> goto endjob;
> @@ -16379,7 +16402,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
> ret = qemuMonitorBlockCommit(priv->mon, device,
> topPath, basePath, backingPath,
> speed);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
Againt ret == 0 possible.
>
> if (mirror) {
> if (ret == 0) {
> @@ -16473,7 +16497,10 @@ qemuDomainOpenGraphics(virDomainPtr dom,
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorOpenGraphics(priv->mon, protocol, fd, "graphicsfd",
> (flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH) != 0);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> + ret = -1;
> + goto cleanup;
We're not going call ObjEndJob?
> + }
> qemuDomainObjEndJob(driver, vm);
>
> cleanup:
> @@ -16542,7 +16569,10 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorOpenGraphics(priv->mon, protocol, pair[1], "graphicsfd",
> (flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH));
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> + ret = -1;
> + goto cleanup;
Again no ObjEndJob?
> + }
> qemuDomainObjEndJob(driver, vm);
> if (ret < 0)
> goto cleanup;
> @@ -16985,7 +17015,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
> goto endjob;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
We could have ret == 0 and goto endjob.
> if (ret < 0)
> goto endjob;
> }
> @@ -17157,7 +17188,8 @@ qemuDomainGetDiskErrors(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> table = qemuMonitorGetBlockInfo(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
> if (!table)
> goto endjob;
>
> @@ -17436,7 +17468,8 @@ qemuDomainPMWakeup(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSystemWakeup(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -17867,7 +17900,8 @@ qemuDomainSetTime(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> rv = qemuMonitorRTCResetReinjection(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
>
> if (rv < 0)
> goto endjob;
> @@ -18512,7 +18546,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> visitBacking);
> ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
> visitBacking));
> - qemuDomainObjExitMonitor(driver, dom);
> + if (qemuDomainObjExitMonitor(driver, dom) < 0)
> + goto cleanup;
>
> if (rc < 0) {
> virResetLastError();
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 31494c8..87b843b 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -550,7 +550,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
> qemuDomainObjPrivatePtr priv = vm->privateData;
> virHashTablePtr stats = NULL;
> size_t i;
> - int ret = -1;
> + int ret = -1, rc;
>
> /* It is not a bug if there already is a NBD data */
> if (!mig->nbd &&
> @@ -571,18 +571,11 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
> goto cleanup;
>
> qemuDomainObjEnterMonitor(driver, vm);
> - if (qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
> - false) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> - }
> - qemuDomainObjExitMonitor(driver, vm);
> -
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("domain exited meanwhile"));
> + if (rc < 0)
This is the kind of construct where I thought a :
if (qemuDomainObjExitMonitor(driver,vm) < 0 || rc < 0)
would fit... It doesn't matter to me - although consistency is nice too.
It's almost like a macro could be generated that would check the status
of qemuMonitor* call and the status of *ExitMonitor using another 'goto'
type parameter in order to consistently handle each of these...
> goto cleanup;
> - }
> }
>
> if (!disk->info.alias ||
> @@ -1655,14 +1648,11 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
> if (!port &&
> ((virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) ||
> (qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))) {
> - qemuDomainObjExitMonitor(driver, vm);
> - goto cleanup;
> + goto exit_monitor;
> }
>
> - if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> - goto cleanup;
> - }
> + if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0)
> + goto exit_monitor;
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> }
> @@ -1675,6 +1665,10 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
> if (ret < 0)
> virPortAllocatorRelease(driver->migrationPorts, port);
> return ret;
> +
> + exit_monitor:
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> + goto cleanup;
> }
>
> /**
> @@ -1764,7 +1758,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
> goto error;
> mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest,
> NULL, speed, 0, 0, mirror_flags);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto error;
>
> if (mon_ret < 0)
> goto error;
> @@ -1785,7 +1780,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
> /* explicitly do this *after* we entered the monitor,
> * as this is a critical section so we are guaranteed
> * priv->job.asyncAbort will not change */
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
> virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
> qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> @@ -1794,7 +1789,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
> }
> mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info,
> NULL);
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
>
> if (mon_ret < 0)
> goto error;
> @@ -1849,7 +1844,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
> BLOCK_JOB_ABORT, true) < 0) {
> VIR_WARN("Unable to cancel block-job on '%s'", diskAlias);
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
The context of this is within a while (lastGood) loop in an error path -
theoretically the failure is dead guest, so what good is contining the
loop? I do agree other paths are mainly ignoring errors too, but it
doesn't seem necessary to continue if the guest died anyway.
> } else {
> VIR_WARN("Unable to enter monitor. No block job cancelled");
> }
> @@ -2231,6 +2226,7 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
> bool wait_for_spice = false;
> bool spice_migrated = false;
> size_t i = 0;
> + int rc;
>
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
> for (i = 0; i < vm->def->ngraphics; i++) {
> @@ -2252,12 +2248,11 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
> QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
> return -1;
>
> - if (qemuMonitorGetSpiceMigrationStatus(priv->mon,
> - &spice_migrated) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + rc = qemuMonitorGetSpiceMigrationStatus(priv->mon, &spice_migrated);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + return -1;
> + if (rc < 0)
> return -1;
> - }
> - qemuDomainObjExitMonitor(driver, vm);
> virObjectUnlock(vm);
> nanosleep(&ts, NULL);
> virObjectLock(vm);
> @@ -2287,7 +2282,8 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
> }
> ret = qemuMonitorGetMigrationStatus(priv->mon, &status);
>
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + return -1;
>
> if (ret < 0 ||
> qemuDomainJobInfoUpdateTime(priv->job.current) < 0)
> @@ -3884,7 +3880,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> /* explicitly do this *after* we entered the monitor,
> * as this is a critical section so we are guaranteed
> * priv->job.asyncAbort will not change */
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
> virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
> qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> @@ -3892,24 +3888,20 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> - if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> - goto cleanup;
> - }
> + if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0)
> + goto exit_monitor;
>
> /* connect to the destination qemu if needed */
> if (spec->destType == MIGRATION_DEST_CONNECT_HOST &&
> qemuMigrationConnect(driver, vm, spec) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> - goto cleanup;
> + goto exit_monitor;
> }
>
> switch (spec->destType) {
> case MIGRATION_DEST_HOST:
> if (STREQ(spec->dest.host.protocol, "rdma") &&
> virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> - goto cleanup;
> + goto exit_monitor;
> }
> ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
> spec->dest.host.protocol,
> @@ -3943,17 +3935,12 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> VIR_FORCE_CLOSE(spec->dest.fd.qemu);
> break;
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
Here is another case where if ret == 0, then goto cleanup doesn't seem
right.
> if (ret < 0)
> goto cleanup;
> ret = -1;
>
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("guest unexpectedly quit"));
> - goto cleanup;
> - }
> -
> /* From this point onwards we *must* call cancel to abort the
> * migration on source if anything goes wrong */
>
> @@ -4045,6 +4032,10 @@ qemuMigrationRun(virQEMUDriverPtr driver,
>
> return ret;
>
> + exit_monitor:
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
should a "ret = -1;" be added here?
I think the rest seem to be OK - my eyes got weary and I hope I didn't
miss any cases. It may be worth going back through the patches 9-13
when you've made all the changes in order to ensure things are more
consistent and that nothing was missed.
ACK with changes -
John
> + goto cleanup;
> +
> cancel:
> orig_err = virSaveLastError();
>
> @@ -4052,7 +4043,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> if (qemuDomainObjEnterMonitorAsync(driver, vm,
> QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
> qemuMonitorMigrateCancel(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> }
> }
> goto cleanup;
> @@ -5330,7 +5321,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
> qemuMonitorSetMigrationSpeed(priv->mon,
> QEMU_DOMAIN_MIG_BANDWIDTH_MAX);
> priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + return -1;
> }
>
> if (!virDomainObjIsActive(vm)) {
> @@ -5406,11 +5398,11 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
> if (virSetCloseExec(pipeFD[1]) < 0) {
> virReportSystemError(errno, "%s",
> _("Unable to set cloexec flag"));
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto cleanup;
> }
> if (virCommandRunAsync(cmd, NULL) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto cleanup;
> }
> rc = qemuMonitorMigrateToFd(priv->mon,
> @@ -5425,14 +5417,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
> args, path, offset);
> }
> }
> - qemuDomainObjExitMonitor(driver, vm);
> -
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("guest unexpectedly quit"));
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> - }
> -
> if (rc < 0)
> goto cleanup;
>
> @@ -5445,7 +5431,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
> if (virDomainObjIsActive(vm) &&
> qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
> qemuMonitorMigrateCancel(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> }
> }
> goto cleanup;
> @@ -5465,7 +5451,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
> qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
> qemuMonitorSetMigrationSpeed(priv->mon, saveMigBandwidth);
> priv->migMaxBandwidth = saveMigBandwidth;
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> }
>
> VIR_FORCE_CLOSE(pipeFD[0]);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list