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

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.

> + /* 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

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

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