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

[Cole Robinson]

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 <eblake at 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 <crobinso at redhat.com>

I don't mind if you adjust and push, or post a v2, or  discuss further, etc

Thanks,
Cole