Re: [PATCH 4/7] tools/utils: introduce xlu_cfg_printf() function

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Thu, Jul 13, 2023 at 07:01:19PM -0700, Elliott Mitchell wrote:
> 
> diff --git a/tools/libs/util/libxlu_cfg.c b/tools/libs/util/libxlu_cfg.c
> index 874f5abfb9..b2947cbfc9 100644
> --- a/tools/libs/util/libxlu_cfg.c
> +++ b/tools/libs/util/libxlu_cfg.c
> @@ -18,12 +18,19 @@
>  #include "libxlu_cfg_i.h"
>  
> +struct XLU_Config {
> +    XLU_ConfigSetting *settings;
> +    FILE *report;
> +    char *config_source;
> +};

When exploring further, for several places config_source being constant
works better.  The single exception is `xlu_cfg_destroy()` which would
then need to cast it to void *.


> @@ -703,6 +710,24 @@ void xlu__cfg_yyerror(YYLTYPE *loc, CfgParseContext 
> *ctx, char const *msg) {
>      if (!ctx->err) ctx->err= EINVAL;
>  }
>  
> +int xlu_cfg_printf(XLU_Config *cfg, const char *format, ...)
> +{
> +     va_list args;
> +     int ret;
> +
> +     if (!cfg || !cfg->report)
> +             return EFAULT;
> +
> +     fwrite(cfg->config_source, 1, strlen(cfg->config_source), cfg->report);
> +     fwrite(": ", 2, 1, cfg->report);

The spots where this doesn't work so well are in libxlu_cfg.c.  Several
spots in libxlu_cfg.c use a format of "%s:%d: <message>" where the %d is
a line number.

Two approaches come to mind to use `xlu_cfg_printf()` for those.  First,
those messages could be modified to include the space.  Second,
`xlu_cfg_printf()` could merely add the colon, but not the space.

I'm pretty sure the upsides and downsides to those approaches are
obvious.  Only issue is which the maintainers would prefer.

(either messages get modified, or else have to add the space in many
places)


> diff --git a/tools/libs/util/libxlu_internal.h 
> b/tools/libs/util/libxlu_internal.h
> index 1f7559ecd9..2ef5eb7f5e 100644
> --- a/tools/libs/util/libxlu_internal.h
> +++ b/tools/libs/util/libxlu_internal.h
> @@ -73,6 +67,9 @@ typedef struct {
>  #define STRINGIFY(x) #x
>  #define TOSTRING(x) STRINGIFY(x)
>  
> +extern int xlu_cfg_printf(XLU_Config *cfg, const char *format, ...)
> +    __attribute__((__format__ (__printf__, 2, 3)));

Again, after a bit more experimentation, the first argument should be
declared constant.

Hopefully 1-3 go through fine though.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445