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

Commit Message

Elliott Mitchell July 14, 2023, 2:01 a.m. UTC

Isolate the lower layer configuration handling from the upper layer.  Now
only the lowest layer of configuration handling looks inside XLU_Config.

Also make error messages a bit more consistent.  Now PCI device parsing
will report filename.

Signed-off-by: Elliott Mitchell <[e-mail]>
---
Someone else can decide where xlu__disk_err() should have its linebreaks
and indentation.  That string isn't very good.

I'm wondering about the return codes.  *printf() can return errors, but
so many places are ignoring them.  If the output is a console the errors
are fairly unlikely, but full storage does happen.
---
 tools/libs/util/libxlu_cfg.c      | 25 +++++++++++++++++++++++++
 tools/libs/util/libxlu_disk.c     | 14 +++++---------
 tools/libs/util/libxlu_internal.h |  9 +++------
 tools/libs/util/libxlu_pci.c      |  3 +--
 tools/libs/util/libxlu_vif.c      |  4 +---
 5 files changed, 35 insertions(+), 20 deletions(-)

Comments

Elliott Mitchell July 19, 2023, 10:06 p.m. UTC | #1

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.

Patch

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 @@ 
 #define _GNU_SOURCE
 
 #include <limits.h>
+#include <stdarg.h>
 
 #include "libxlu_internal.h"
 #include "libxlu_cfg_y.h"
 #include "libxlu_cfg_l.h"
 #include "libxlu_cfg_i.h"
 
+struct XLU_Config {
+    XLU_ConfigSetting *settings;
+    FILE *report;
+    char *config_source;
+};
+
 XLU_Config *xlu_cfg_init(FILE *report, const char *report_source) {
     XLU_Config *cfg;
 
@@ -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);
+
+	va_start(args, format);
+	ret = vfprintf(cfg->report, format, args);
+	va_end(args);
+
+	return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/util/libxlu_disk.c b/tools/libs/util/libxlu_disk.c
index 1de16a6a06..a4d08ab7e9 100644
--- a/tools/libs/util/libxlu_disk.c
+++ b/tools/libs/util/libxlu_disk.c
@@ -5,13 +5,10 @@ 
 
 void xlu__disk_err(DiskParseContext *dpc, const char *erroneous,
                    const char *message) {
-    fprintf(dpc->cfg->report,
-            "%s: config parsing error in disk specification: %s"
-            "%s%s%s"
-            " in `%s'\n",
-            dpc->cfg->config_source, message,
-            erroneous?": near `":"", erroneous?erroneous:"", erroneous?"'":"",
-            dpc->spec);
+    xlu_cfg_printf(dpc->cfg,
+            "config parsing error in disk specification: %s%s%s%s in `%s'\n",
+            message, erroneous?": near `":"", erroneous?erroneous:"",
+            erroneous?"'":"", dpc->spec);
     if (!dpc->err) dpc->err= EINVAL;
 }
 
@@ -29,8 +26,7 @@  static int dpc_prep(DiskParseContext *dpc, const char *spec) {
     return 0;
 
  fail:
-    fprintf(dpc->cfg->report, "cannot init disk scanner: %s\n",
-            strerror(errno));
+    xlu_cfg_printf(dpc->cfg, "cannot init disk scanner: %s\n", strerror(errno));
     return e;
 }
 
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
@@ -57,12 +57,6 @@  typedef struct XLU_ConfigSetting { /* transparent */
     int lineno;
 } XLU_ConfigSetting;
 
-struct XLU_Config {
-    XLU_ConfigSetting *settings;
-    FILE *report;
-    char *config_source;
-};
-
 typedef struct {
     XLU_Config *cfg;
     int err, lexerrlineno, likely_python;
@@ -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)));
+
 #endif /*LIBXLU_INTERNAL_H*/
 
 /*
diff --git a/tools/libs/util/libxlu_pci.c b/tools/libs/util/libxlu_pci.c
index 294482c6d7..d6abbc1c1f 100644
--- a/tools/libs/util/libxlu_pci.c
+++ b/tools/libs/util/libxlu_pci.c
@@ -5,8 +5,7 @@ 
 #include "libxlu_internal.h"
 
 
-#define XLU__PCI_ERR(_c, _x, _a...) \
-    if((_c) && (_c)->report) fprintf((_c)->report, _x, ##_a)
+#define XLU__PCI_ERR(_c, _x, _a...) xlu_cfg_printf((_c), _x, ##_a)
 
 static int parse_bdf(libxl_device_pci *pci, const char *str, const char **endp)
 {
diff --git a/tools/libs/util/libxlu_vif.c b/tools/libs/util/libxlu_vif.c
index ccf0cbdf57..93c449e213 100644
--- a/tools/libs/util/libxlu_vif.c
+++ b/tools/libs/util/libxlu_vif.c
@@ -6,9 +6,7 @@  static const char *vif_bytes_per_sec_re = "^[0-9]+[GMK]?[Bb]/s$";
 static const char *vif_internal_usec_re = "^[0-9]+[mu]?s?$";
 
 static void xlu__vif_err(XLU_Config *cfg, const char *msg, const char *rate) {
-    fprintf(cfg->report,
-            "%s: config parsing error in vif: %s in `%s'\n",
-            cfg->config_source, msg, rate);
+    xlu_cfg_printf(cfg, "config parsing error in vif: %s in `%s'\n", msg, rate);
 }
 
 static int vif_parse_rate_bytes_per_sec(XLU_Config *cfg, const char *bytes,