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@xxxxxxxxxx>
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
