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.
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> src/conf/domain_conf.h | 9 +++++++++
> src/conf/domain_conf.c | 24 ++++++++++++++++++++++++
> src/conf/snapshot_conf.c | 2 +-
> src/libvirt_private.syms | 1 +
> 4 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 988ef90f11..be3b8bedf2 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2693,6 +2693,15 @@ virDomainXMLOptionPtr
> virDomainXMLOptionNew(virDomainDefParserConfigPtr config,
> virSaveCookieCallbacksPtr
> virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt);
>
> +typedef int (*virDomainMomentPostParseCallback)(virDomainMomentDefPtr def,
> + void *opaque);
> +
> +void virDomainXMLOptionSetMomentPostParse(virDomainXMLOptionPtr xmlopt,
> + virDomainMomentPostParseCallback
> cb,
> + void *opaque);
> +int virDomainXMLOptionRunMomentPostParse(virDomainXMLOptionPtr xmlopt,
> + virDomainMomentDefPtr def);
> +
> void virDomainNetGenerateMAC(virDomainXMLOptionPtr xmlopt, virMacAddrPtr
> mac);
>
> virDomainXMLNamespacePtr
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 17e8975be3..32d4539f69 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -82,6 +82,10 @@ struct _virDomainXMLOption {
>
> /* Private data for save image stored in snapshot XML */
> virSaveCookieCallbacks saveCookie;
> +
> + /* 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 <crobi...@redhat.com>
I don't mind if you adjust and push, or post a v2, or discuss further, etc
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
