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

[Jim Fehlig]

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 at oracle.com>
>>> ---
>>> 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