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

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(a)oracle.com&gt; >> --- >> 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?