Re: [PATCH 04/10] remote_driver: Expose EXTRACT_URI_ARG_* macros

On Mon, Feb 06, 2023 at 10:16:52 +0100, Michal Privoznik wrote: > Almost in all places where an URI is parsed we look for > additional argument(s). The remote driver's parsing uses two > macros EXTRACT_URI_ARG_STR() and EXTRACT_URI_ARG_BOOL() for that > purpose. Expose these so that other places can be rewritten using > those macros. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > po/POTFILES | 1 + > src/remote/remote_driver.c | 58 +++++++++++--------------------------- > src/util/viruri.h | 23 +++++++++++++++ > 3 files changed, 40 insertions(+), 42 deletions(-) [...] > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index 6a226999df..c41d5b414f 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -693,30 +693,6 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn, > return rc != -1 && ret.supported; > } > > -/* helper macro to ease extraction of arguments from the URI */ > -#define EXTRACT_URI_ARG_STR(ARG_NAME, ARG_VAR) \ > - if (STRCASEEQ(var->name, ARG_NAME)) { \ > - VIR_FREE(ARG_VAR); \ > - ARG_VAR = g_strdup(var->value); \ > - var->ignore = 1; \ > - continue; \ > - } This style of macros, which apart from arguments also accesses variables directly from the place where it's expanded or influence control flow ... > @@ -796,23 +772,23 @@ doRemoteOpen(virConnectPtr conn, > if (conn->uri) { > for (i = 0; i < conn->uri->paramsCount; i++) { > virURIParam *var = &conn->uri->params[i]; > - EXTRACT_URI_ARG_STR("name", name); > - EXTRACT_URI_ARG_STR("command", command); > - EXTRACT_URI_ARG_STR("socket", sockname); > - EXTRACT_URI_ARG_STR("auth", authtype); ... are really acceptable only in ad-hoc situations as here ... > @@ -1206,8 +1182,6 @@ doRemoteOpen(virConnectPtr conn, > VIR_FREE(priv->hostname); > return VIR_DRV_OPEN_ERROR; > } > -#undef EXTRACT_URI_ARG_STR > -#undef EXTRACT_URI_ARG_BOOL ... where we dispose of it right away. > > static struct private_data * > remoteAllocPrivateData(void) > diff --git a/src/util/viruri.h b/src/util/viruri.h > index 4f27fa26d2..0e4176c037 100644 > --- a/src/util/viruri.h > +++ b/src/util/viruri.h > @@ -62,3 +62,26 @@ const char *virURIGetParam(virURI *uri, const char *name); > bool virURICheckUnixSocket(virURI *uri); > > #define VIR_URI_SERVER(uri) ((uri) && (uri)->server ? (uri)->server : "localhost") > + > +/* helper macros to ease extraction of arguments from the URI */ > +#define VIR_EXTRACT_URI_ARG_STR(ARG_NAME, ARG_VAR) \ Exporting it as such is not acceptable. Less so with non-existent docs outlining the "side effects" not visible from the place where it's called. I suggest you turn it into a function (keeping proper naming scheme) such as: int virURIParamExtractStr(virURIParam *par, const char *name, char **val) { if (STRCASEEQ(par->name, name)) { g_free(*val); *val = g_strdup(par->value); par->ignore = true; return 1; } return 0; } Alternatively return a bool. And use it as: if (virURIParamExtractStr(var, "name", &name) == 1) continue;