Re: [PATCH v2 2/3] libxl: streamline top-level migrate functions

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 02/14/2017 04:44 AM, Joao Martins wrote:
> On 02/14/2017 03:13 AM, Jim Fehlig wrote:
> > On 02/07/2017 05:35 PM, Joao Martins wrote:
> > > This allows us to reuse a single function for both tunnelled and
> > > non-tunnelled variants.
> > >
> > > Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> > > ---
> > > New in v2
> > > ---
> > >  src/libxl/libxl_driver.c | 36 +++++++++++++++++++++++++++---------
> > >  1 file changed, 27 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > > index 3a69720..7bc8adf 100644
> > > --- a/src/libxl/libxl_driver.c
> > > +++ b/src/libxl/libxl_driver.c
> > > @@ -5930,21 +5930,22 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
> > >  }
> > >
> > >  static int
> > > -libxlDomainMigratePrepare3Params(virConnectPtr dconn,
> > > -                                 virTypedParameterPtr params,
> > > -                                 int nparams,
> > > -                                 const char *cookiein,
> > > -                                 int cookieinlen,
> > > -                                 char **cookieout ATTRIBUTE_UNUSED,
> > > -                                 int *cookieoutlen ATTRIBUTE_UNUSED,
> > > -                                 char **uri_out,
> > > -                                 unsigned int flags)
> > > +libxlDomainMigratePrepareCommon(virConnectPtr dconn,
> > > +                                virTypedParameterPtr params,
> > > +                                int nparams,
> > > +                                const char *cookiein,
> > > +                                int cookieinlen,
> > > +                                char **cookieout ATTRIBUTE_UNUSED,
> > > +                                int *cookieoutlen ATTRIBUTE_UNUSED,
> > > +                                unsigned int flags,
> > > +                                void *data)
> > >  {
> > >      libxlDriverPrivatePtr driver = dconn->privateData;
> > >      virDomainDefPtr def = NULL;
> > >      const char *dom_xml = NULL;
> > >      const char *dname = NULL;
> > >      const char *uri_in = NULL;
> > > +    char **uri_out = data;
> > >
> > >  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
> > >      virReportUnsupportedError();
> > > @@ -5985,6 +5986,23 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn,
> > >  }
> > >
> > >  static int
> > > +libxlDomainMigratePrepare3Params(virConnectPtr dconn,
> > > +                                 virTypedParameterPtr params,
> > > +                                 int nparams,
> > > +                                 const char *cookiein,
> > > +                                 int cookieinlen,
> > > +                                 char **cookieout ATTRIBUTE_UNUSED,
> > > +                                 int *cookieoutlen ATTRIBUTE_UNUSED,
> > > +                                 char **uri_out,
> > > +                                 unsigned int flags)
> > > +{
> > > +    return libxlDomainMigratePrepareCommon(dconn, params, nparams,
> > > +                                           cookiein, cookieinlen,
> > > +                                           cookieout, cookieoutlen,
> > > +                                           flags, uri_out);
> > > +}
> >
> > It appears the ACL check must be done in libxlDomainMigratePrepare3Params to
> > satisfy 'make check'
> >
> > ./libxl/libxl_driver.c:5978 Mismatch check
> > 'virDomainMigratePrepare3ParamsEnsureACL' for function
> > 'libxlDomainMigratePrepareCommon'
>
> Sorry, I naively ran solely the syntax checks when rebasing this series.
>
> The ACL checks require dconn and def, and consequently the call to fetch def:
>
> def = libxlDomainMigrationPrepareDef(driver, dom_xml, dname);
>
> Requires dom_xml and dname being fetched from params.

:-(

> Given all these dependencies having this common function for both prepare
> functions doesn't seem to be doing much, as the top-level functions would end up
> being very similar after these dependencies. Which makes me wonder if we should
> dropped this (i.e. remove this PrepareCommon function) instead and go like
> initial v1 (same comment would be applicable for the PrepareTunnel3Params in
> patch 3). What do you think?

Agree. Drop this and go with v1 approach of separate functions.

Regards,
Jim

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list