Re: [libvirt] [PATCHv2 12/14] Exit early after domain crash in monitor in qemu_driver

<no commit message>? On 01/07/2015 10:42 AM, Ján Tomko wrote: Should qemu_migration move to 10/14? Here we have another Audit issue... if ret == 0, then @ cleanup we return success Hmmm... do we need to call DomainObjEndJob if ExitMonitor fails? I don't even remember if this was an issue for previous such exits... again, if ret == 0, then we return wrong status possibly... Also I see another EndJob below... > @@ -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... 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; > @@ -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; > @@ -12457,7 +12476,8 @@ qemuDomainMigrateGetCompressionCache(virDomainPtr dom, > ret = qemuMonitorGetMigrationCacheSize(priv->mon, cacheSize); > } > > - qemuDomainObjExitMonitor(driver, vm); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + ret = -1; > @@ -12511,7 +12531,8 @@ qemuDomainMigrateSetCompressionCache(virDomainPtr dom, > ret = qemuMonitorSetMigrationCacheSize(priv->mon, cacheSize); > } > > - qemuDomainObjExitMonitor(driver, vm); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + ret = -1; > @@ -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. > + } > @@ -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. 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... 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? Another ret == 0 possibility OK - here we Audit right after the DriveMirror, then exit the Monitor... Another place where ret == 0 could be true... Againt ret == 0 possible. We're not going call ObjEndJob? Again no ObjEndJob? 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; > @@ -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... 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. Here is another case where if ret == 0, then goto cleanup doesn't seem right. 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