On 01/15/2014 07:32 AM, John Ferlan wrote: > > > On 01/14/2014 04:43 PM, Eric Blake wrote: >> v2 of my patch series started here, after applying all patches >> already reviewed in that thread: >> https://www.redhat.com/archives/libvir-list/2013-December/msg01284.html >> >> Patches 1-3 can be considered bug fixes (particularly patch 3), >> so I'd like them in 1.2.1 if they get a favorable review. Patches >> 4-6 are less urgent, but might as well finish the work all within >> one release. >> >> Patch 1 comes from 5/24 in v1 and I squashed in the fix pointed out by Jan. > > Patch 3 seems to be the one where there would be a couple of migration > fixes that could use a backport, especially the change in > virDomainMigrateVersion3Full() where the code now actually honors the > condition where the uri was not set rather than just playing dumb. I > also keep looking the changed "single" (!ret) to a "multi-state" (!ret > && !xxx) condition and keep asking myself could we run into a situation > where (!ret and xxx) is true where previously we'd fail on the !ret, but > now wouldn't because xxx was true (where xxx is retcode or cancelled). Yes - the scenario is as follows: pre-patch: public entry point virDomainMigrate() -> static virDomainMigrateVersion2() -> during prepare phase, fails to get URI, dispatches an error then goes to finish -> in finish phase, calls domainMigrateFinish2(cancelled = 1) -> MigrateFinish2() will return NULL (because cancelled was set - if it doesn't return NULL, that's a bug, where I added VIR_WARN to flag it), but without setting an error -> because it returns NULL, we goto the error label -> the error label tries to dispatch the error post-patch, we realize that if we are calling MigrateFinish2() on the cleanup path of an aborted migration attempt (if cancelled/retcode is non-zero), then the error message earlier in the 5-step process that caused migration to be cancelled is the important error, and we really don't care what happens in the finish step. That is, we want to return NULL without raising an error if we know we are calling the finish step on a cleanup path, and only use the 'goto error' label in the API when we are in the finish step with all earlier steps of migration successful so far. > > Another "step" to consider for patch 5 would be to make common the > repetitive code that follows the virDriverCheckTabMaxReturn(), e.g. > > VIR_DEBUG('string') > table[count] = driver > return count++ I didn't think it was worth the further compression. > > I don't see a reason other than proximity to release date to preclude > inclusion in 1.2.1 So more to the point, should I push now, or wait until post-release? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list