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

[Daniel P. Berrangé]

On Tue, Feb 07, 2023 at 09:05:36AM +0100, Peter Krempa wrote:
> 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 at redhat.com>
> > ---
> >  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;

Alternatively declare this code all redundant optimizatio and
just use the existing virURIGetParam instead. It'll iterate
over the parameters many times, but that's irrelevant when
we normally have zero or one parameters :-)


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|