Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog: Provide a default logger
Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog:
Provide a default logger"):
> Although actually, you can't capture stderr in a variable like this at
> all. freopen() at a later point will close the current FILE object and
> create a new one, after which this logger will use-after-free.
freopen(...,f) always returns NULL or f. There's no way to change the
pointer value of stderr. Indeed some libcs have
#define stderr (&__stdiostreams[2])
or some such.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog: Provide a default logger
On 08/11/18 17:31, Andrew Cooper wrote:
> On 08/11/18 17:28, Ian Jackson wrote:
>> Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog:
>> Provide a default logger"):
>>> 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;
>>> }
>> Blimey. Is that portable enough ?
> Should be. Its how C++ globals work, and it appears that we already use
> it in xc_dom.h for the dombuilder register_{loader,arch_hooks}()
> infrastructure.
Although actually, you can't capture stderr in a variable like this at
all. freopen() at a later point will close the current FILE object and
create a new one, after which this logger will use-after-free.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog: Provide a default logger
On 08/11/18 17:28, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog:
> Provide a default logger"):
>> 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;
>> }
> Blimey. Is that portable enough ?
Should be. Its how C++ globals work, and it appears that we already use
it in xc_dom.h for the dombuilder register_{loader,arch_hooks}()
infrastructure.
> I can switch to that if we think so. It's certainly more pleasant.
Absolutely (although be warned - that was entirely untested code).
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog: Provide a default logger
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
> CC: Wei Liu
> ---
> 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
> #include
> #include
> +#include
>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog: Provide a default logger
Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 01/11] tools/libs/toollog:
Provide a default logger"):
> 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;
> }
Blimey. Is that portable enough ? I can switch to that if we think
so. It's certainly more pleasant.
> 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).
Oops, although in practice I think this is not going to make any
difference on any supported platform.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 01/11] tools/libs/toollog: Provide a default logger
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
CC: Wei Liu
---
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
#include
#include
+#include
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.
+ */
+static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+pthread_mutex_lock();
+deflogger.f = stderr;
+pthread_mutex_unlock();
+
+return
+};
+
/*
* Local variables:
* mode: C
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
