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

[Peter Krempa <pkrempa@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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;

> +    if (STRCASEEQ(var->name, ARG_NAME)) { \
> +        g_free(ARG_VAR); \
> +        ARG_VAR = g_strdup(var->value); \
> +        var->ignore = 1; \
> +        continue; \
> +    }