[libvirt] [PATCHv2 0/6] remaining cleanups to libvirt.c

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140115/2cc72b02/attachment-0001.sig>