Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog: Provide a default logger

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/11/18 17:07, Ian Jackson wrote:
> This is most conveniently done like this because xtl_logger_stdio.c
> knows how to provide a static logger without doing any memory
> allocations.  That's useful because it can't fail.
>
> Add the new symbol to the map file and bump the minor version
> accordingly.
>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> v2: New in this version of the series
> ---
>  tools/libs/toollog/Makefile             |  2 +-
>  tools/libs/toollog/include/xentoollog.h |  5 +++++
>  tools/libs/toollog/libxentoollog.map    |  5 +++++
>  tools/libs/toollog/xtl_logger_stdio.c   | 26 ++++++++++++++++++++++++++
>  4 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libs/toollog/Makefile b/tools/libs/toollog/Makefile
> index 8aae2c8f53..3aa0997757 100644
> --- a/tools/libs/toollog/Makefile
> +++ b/tools/libs/toollog/Makefile
> @@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
>  include $(XEN_ROOT)/tools/Rules.mk
>  
>  MAJOR        = 1
> -MINOR        = 0
> +MINOR        = 1
>  SHLIB_LDFLAGS += -Wl,--version-script=libxentoollog.map
>  
>  CFLAGS       += -Werror -Wmissing-prototypes
> diff --git a/tools/libs/toollog/include/xentoollog.h 
> b/tools/libs/toollog/include/xentoollog.h
> index 76f17fe125..942eb76169 100644
> --- a/tools/libs/toollog/include/xentoollog.h
> +++ b/tools/libs/toollog/include/xentoollog.h
> @@ -86,6 +86,11 @@ void 
> xtl_stdiostream_adjust_flags(xentoollog_logger_stdiostream*,
>  void xtl_logger_destroy(struct xentoollog_logger *logger /* 0 is ok */);
>  
>  
> +xentoollog_logger_stdiostream *xtl_defaultlogger_stdiostream(void);
> +  /* Returns pointer to a static global logger which writes to stderr.
> +   * Reconfiguring it is permitted but destroying it is forbidden.
> +   * This function cannot fail. */
> +
>  /*---------- facilities for generating log messages ----------*/
>  
>  void xtl_logv(struct xentoollog_logger *logger,
> diff --git a/tools/libs/toollog/libxentoollog.map 
> b/tools/libs/toollog/libxentoollog.map
> index c183cf555d..00eaacaeaf 100644
> --- a/tools/libs/toollog/libxentoollog.map
> +++ b/tools/libs/toollog/libxentoollog.map
> @@ -10,3 +10,8 @@ VERS_1.0 {
>               xtl_stdiostream_set_minlevel;
>       local: *; /* Do not expose anything by default */
>  };
> +
> +VERS_1.1 {
> +     global:
> +             xtl_defaultlogger_stdiostream;
> +} VERS_1.0;
> diff --git a/tools/libs/toollog/xtl_logger_stdio.c 
> b/tools/libs/toollog/xtl_logger_stdio.c
> index 52dfbf51e3..07fe355626 100644
> --- a/tools/libs/toollog/xtl_logger_stdio.c
> +++ b/tools/libs/toollog/xtl_logger_stdio.c
> @@ -28,6 +28,7 @@
>  #include <stdlib.h>
>  #include <errno.h>
>  #include <stdbool.h>
> +#include <pthread.h>
>  
>  struct xentoollog_logger_stdiostream {
>      xentoollog_logger vtable;
> @@ -191,6 +192,31 @@ xentoollog_logger_stdiostream 
> *xtl_createlogger_stdiostream
>      return XTL_NEW_LOGGER(stdiostream, newlogger);
>  }
>  
> +xentoollog_logger_stdiostream *xtl_defaultlogger_stdiostream(void) {
> +    static xentoollog_logger_stdiostream deflogger = {
> +        .vtable = {
> +            .vmessage = stdiostream_vmessage,
> +            .progress = stdiostream_progress,
> +            .destroy  = 0, /* no-one should destroy this */
> +        },
> +        .min_level = XTL_PROGRESS,
> +        /* for other fields except .f, 0 is good */
> +    };
> +
> +    /*
> +     * Unfortunately, stderr is not a `constant expression', so we
> +     * can't handle it in the initialisation.  Also we can't do a
> +     * lockless assignment, even of the identical value, without
> +     * violating threading rules.  Nnng.

You want something like:

static xentoollog_logger_stdiostream stdio_logger = {
    .vtable = {
        .vmessage = stdiostream_vmessage,
        .progress = stdiostream_progress,
        .destroy  = 0, /* no-one should destroy this */
    },
    .min_level = XTL_PROGRESS,
    /* for other fields except .f, 0 is good */
};

static void __attribute__((__constructor__)) init_stdio_logger(void)
{
    stdio_logger.f = stderr;
}

Which will cause the library loader to DTRT, but not require you to link
against pthread (which is a latent bug here, as you didn't update the
SHDEPS).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel