[libvirt] [PATCH 4/5] snapshot: Allow for post-parse override

[Eric Blake]

On 4/16/19 1:54 PM, Cole Robinson wrote:
> On 4/16/19 9:53 AM, Eric Blake wrote:
>> Wire up the accessor functions necessary for the testsuite to install
>> an alternative post-parse handler from normal drivers. I could have
>> modified the signature for virDomainXMLOptionNew() to take another
>> parameter, but thought it was easier to add a new set function rather
>> than chase down all existing callers. Until code actually sets the
>> override, there is no change in behavior.
>>
> 
> I agree that extending DomainXMLOptionNew with yet another parameter
> would be a pain, I like this pattern better.

Yay, glad I picked it right, then :)


>> +    /* Snapshot postparse callbacks */
>> +    virDomainMomentPostParseCallback moment;
>> +    void *moment_opaque;
>>  };
>>
>>  #define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS \
>> @@ -1451,6 +1455,26 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt)
>>  }
>>
>>
>> +void
>> +virDomainXMLOptionSetMomentPostParse(virDomainXMLOptionPtr xmlopt,
>> +                                     virDomainMomentPostParseCallback cb,
>> +                                     void *opaque)
>> +{
>> +    xmlopt->moment = cb;
>> +    xmlopt->moment_opaque = opaque;
>> +}
>> +
> 
> Calling it 'moment' isn't very clear. The domain equivalent is
> domainPostParseCallback so I suggest momentPostParseCallback

Renaming is easy, I'll go with the longer name.

> 
> The moment_opaque pattern is weird to me, setting a value like that my
> first thought is it should be freed. And the stock MomentPostParse
> having a different signature than the callback confused me
> 
> In the following patch the only usage is passing in 'timeptr' which is
> already global, can we drop moment_opaque and just access timeptr
> directly? If later usage actually needs to pass in opaque data, it
> should be modeled more like virDomainPostParse IMO

Works for me. It feels a bit dirty depending on global state, but it's
just a testsuite.

> 
> Otherwise, for the series:
> 
> Reviewed-by: Cole Robinson <crobinso at redhat.com>
> 
> I don't mind if you adjust and push, or post a v2, or  discuss further, etc

I think I'll go ahead and just make the modifications you requested,
then push; I'm making the same changes to my checkpoint code, where I
hope to post v8 later today.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org