[PATCH v2 00/11] xentoollog, libvchan: Minor improvements

[PATCH v2 00/11] xentoollog, libvchan: Minor improvements

[Ian Jackson]

The point of this series is to make an API promise about what ENOENT
means from libxenvchan_client_init, to make it easier to build async
code on top of this (eg, libxl).

There is a lot of internal tidying, but also a necessary prefix is a
change to xentoollog to make it OK to pass NULL for a logger.  This
allows elimination of all the duplicates of the default logger creation.
(Otherwise we'd have to have added yet another dupe to libvchan.)

 +  01/11  tools/libs/toollog: Provide a default logger
 +  02/11  tools/libs/toollog: Use the default logger
 +  03/11  tools/libs/*: Rely on the default logger
  a 04/11  tools/libvchan: Initialise xs_transaction_t to XBT_NULL, not NULL
  a 05/11  tools/xenstore: Document that xs_close(0) is OK.
  a 06/11  tools/libvchan: init_xs_srv: Simplify error handling (1)
  a 07/11  tools/libvchan: init_xs_srv: Simplify error handling (2)
  a 08/11  tools/libvchan: init_xs_srv: Turn xs retry from goto into for (;;)
  a 09/11  tools/libvchan: Add xentoollog to direct dependencies
  a 10/11  tools/libvchan: libxenvchan_*_init: Promise an errno
    11/11  tools/libvchan: libxenvchan_client_init: use ENOENT for no server

  +  new patch in v2
  a  acked by tools maintainer

 tools/libs/call/core.c                  | 10 -----
 tools/libs/call/private.h               |  2 +-
 tools/libs/devicemodel/core.c           | 11 -----
 tools/libs/devicemodel/private.h        |  2 +-
 tools/libs/evtchn/core.c                | 10 -----
 tools/libs/evtchn/private.h             |  2 +-
 tools/libs/foreignmemory/core.c         | 10 -----
 tools/libs/foreignmemory/private.h      |  2 +-
 tools/libs/gnttab/gntshr_core.c         | 10 -----
 tools/libs/gnttab/gnttab_core.c         | 10 -----
 tools/libs/gnttab/private.h             |  2 +-
 tools/libs/toollog/Makefile             |  2 +-
 tools/libs/toollog/include/xentoollog.h | 12 ++++--
 tools/libs/toollog/libxentoollog.map    |  5 +++
 tools/libs/toollog/xtl_core.c           |  7 ++++
 tools/libs/toollog/xtl_logger_stdio.c   | 26 ++++++++++++
 tools/libvchan/Makefile                 |  6 +--
 tools/libvchan/init.c                   | 72 +++++++++++++++++++--------------
 tools/libvchan/libxenvchan.h            |  8 +++-
 tools/libvchan/xenvchan.pc.in           |  2 +-
 tools/xenstore/include/xenstore.h       |  2 +-
 21 files changed, 105 insertions(+), 108 deletions(-)

-- 
2.11.0


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

[PATCH v2 01/11] tools/libs/toollog: Provide a default logger

[Ian Jackson]

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@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
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.
+     */
+    static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+    pthread_mutex_lock(&mutex);
+    deflogger.f = stderr;
+    pthread_mutex_unlock(&mutex);
+
+    return &deflogger;
+};
+
 /*
  * Local variables:
  * mode: C
-- 
2.11.0


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

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

[Andrew Cooper]

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@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Ian Jackson]

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

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

[Andrew Cooper]

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: [PATCH v2 01/11] tools/libs/toollog: Provide a default logger

[Andrew Cooper]

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: [PATCH v2 01/11] tools/libs/toollog: Provide a default logger

[Ian Jackson]

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

[PATCH v2 02/11] tools/libs/toollog: Use the default logger

[Ian Jackson]

Previously xtl_log, xtl_logv and xtl_progress would all crash if
passed logger=NULL.  Have the use the default logger instead.
This is more convenient.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
v2: New in this version of the series
---
 tools/libs/toollog/include/xentoollog.h | 9 +++++----
 tools/libs/toollog/xtl_core.c           | 7 +++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/libs/toollog/include/xentoollog.h b/tools/libs/toollog/include/xentoollog.h
index 942eb76169..9c7725f1cd 100644
--- a/tools/libs/toollog/include/xentoollog.h
+++ b/tools/libs/toollog/include/xentoollog.h
@@ -89,25 +89,26 @@ 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. */
+   * This function cannot fail.
+   * This default logger is usedd by xtl_log et al when logger==NULL. */
 
 /*---------- facilities for generating log messages ----------*/
 
-void xtl_logv(struct xentoollog_logger *logger,
+void xtl_logv(struct xentoollog_logger *logger /* NULL ok */,
               xentoollog_level level,
               int errnoval /* or -1 */,
               const char *context /* eg "xc", "xenstore", "xl", may be 0 */,
               const char *format /* does not contain \n */,
               va_list) __attribute__((format(printf,5,0)));
 
-void xtl_log(struct xentoollog_logger *logger,
+void xtl_log(struct xentoollog_logger *logger /* NULL ok */,
              xentoollog_level level,
              int errnoval /* or -1 */,
              const char *context /* eg "xc", "xenstore", "xl" */,
              const char *format /* does not contain \n */,
              ...) __attribute__((format(printf,5,6)));
 
-void xtl_progress(struct xentoollog_logger *logger,
+void xtl_progress(struct xentoollog_logger *logger /* NULL ok */,
                   const char *context /* see above, may be 0 */,
                   const char *doing_what,
                   unsigned long done, unsigned long total);
diff --git a/tools/libs/toollog/xtl_core.c b/tools/libs/toollog/xtl_core.c
index 099d2f3095..c5da3f4f5d 100644
--- a/tools/libs/toollog/xtl_core.c
+++ b/tools/libs/toollog/xtl_core.c
@@ -38,6 +38,11 @@ const char *xtl_level_to_string(xentoollog_level level) {
     return level_strings[level];
 }
 
+static void defaultlogger(xentoollog_logger **logger) {
+    if (*logger) return;
+    *logger = (xentoollog_logger*)xtl_defaultlogger_stdiostream();
+}
+
 void xtl_logv(struct xentoollog_logger *logger,
               xentoollog_level level,
               int errnoval /* or -1 */,
@@ -46,6 +51,7 @@ void xtl_logv(struct xentoollog_logger *logger,
               va_list al) {
     int errno_save = errno;
     assert(level > XTL_NONE && level < XTL_NUM_LEVELS);
+    defaultlogger(&logger);
     logger->vmessage(logger,level,errnoval,context,format,al);
     errno = errno_save;
 }
@@ -67,6 +73,7 @@ void xtl_progress(struct xentoollog_logger *logger,
                   unsigned long done, unsigned long total) {
     int percent = 0;
 
+    defaultlogger(&logger);
     if (!logger->progress) return;
 
     if ( total )
-- 
2.11.0


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

Re: [PATCH v2 02/11] tools/libs/toollog: Use the default logger

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 3937 bytes --]

On Thu, Nov 08, 2018 at 05:07:56PM +0000, Ian Jackson wrote:
> Previously xtl_log, xtl_logv and xtl_progress would all crash if
> passed logger=NULL.  Have the use the default logger instead.
> This is more convenient.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
> v2: New in this version of the series
> ---
>  tools/libs/toollog/include/xentoollog.h | 9 +++++----
>  tools/libs/toollog/xtl_core.c           | 7 +++++++
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libs/toollog/include/xentoollog.h b/tools/libs/toollog/include/xentoollog.h
> index 942eb76169..9c7725f1cd 100644
> --- a/tools/libs/toollog/include/xentoollog.h
> +++ b/tools/libs/toollog/include/xentoollog.h
> @@ -89,25 +89,26 @@ 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. */
> +   * This function cannot fail.
> +   * This default logger is usedd by xtl_log et al when logger==NULL. */
>  
>  /*---------- facilities for generating log messages ----------*/
>  
> -void xtl_logv(struct xentoollog_logger *logger,
> +void xtl_logv(struct xentoollog_logger *logger /* NULL ok */,
>                xentoollog_level level,
>                int errnoval /* or -1 */,
>                const char *context /* eg "xc", "xenstore", "xl", may be 0 */,
>                const char *format /* does not contain \n */,
>                va_list) __attribute__((format(printf,5,0)));
>  
> -void xtl_log(struct xentoollog_logger *logger,
> +void xtl_log(struct xentoollog_logger *logger /* NULL ok */,
>               xentoollog_level level,
>               int errnoval /* or -1 */,
>               const char *context /* eg "xc", "xenstore", "xl" */,
>               const char *format /* does not contain \n */,
>               ...) __attribute__((format(printf,5,6)));
>  
> -void xtl_progress(struct xentoollog_logger *logger,
> +void xtl_progress(struct xentoollog_logger *logger /* NULL ok */,
>                    const char *context /* see above, may be 0 */,
>                    const char *doing_what,
>                    unsigned long done, unsigned long total);
> diff --git a/tools/libs/toollog/xtl_core.c b/tools/libs/toollog/xtl_core.c
> index 099d2f3095..c5da3f4f5d 100644
> --- a/tools/libs/toollog/xtl_core.c
> +++ b/tools/libs/toollog/xtl_core.c
> @@ -38,6 +38,11 @@ const char *xtl_level_to_string(xentoollog_level level) {
>      return level_strings[level];
>  }
>  
> +static void defaultlogger(xentoollog_logger **logger) {
> +    if (*logger) return;
> +    *logger = (xentoollog_logger*)xtl_defaultlogger_stdiostream();
> +}
> +
>  void xtl_logv(struct xentoollog_logger *logger,
>                xentoollog_level level,
>                int errnoval /* or -1 */,
> @@ -46,6 +51,7 @@ void xtl_logv(struct xentoollog_logger *logger,
>                va_list al) {
>      int errno_save = errno;
>      assert(level > XTL_NONE && level < XTL_NUM_LEVELS);
> +    defaultlogger(&logger);
>      logger->vmessage(logger,level,errnoval,context,format,al);
>      errno = errno_save;
>  }
> @@ -67,6 +73,7 @@ void xtl_progress(struct xentoollog_logger *logger,
>                    unsigned long done, unsigned long total) {
>      int percent = 0;
>  
> +    defaultlogger(&logger);
>      if (!logger->progress) return;
>  
>      if ( total )

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2 02/11] tools/libs/toollog: Use the default logger

[Wei Liu]

On Thu, Nov 08, 2018 at 05:07:56PM +0000, Ian Jackson wrote:
> Previously xtl_log, xtl_logv and xtl_progress would all crash if
> passed logger=NULL.  Have the use the default logger instead.
> This is more convenient.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v2 03/11] tools/libs/*: Rely on the default logger

[Ian Jackson]

Delete 11 entirely formulaic conditional calls to
  xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
and associated logger_tofree variables, error handling, etc.

No overall functional change, although some memory allocation errors
may no longer occur.

After this there are still several calls to
xtl_createlogger_stdiostream in tree, but they almost all have
non-default message level etc. so it is not obvious that they should
be replaced.

The exception is in xc_private.c where xch->error_handler is
initialised using a copy of the default initialisation boilerplate
(ant there is the associated xch->error_handler_tofree).  However,
there is also xch->dombuild_logger, and xch->dombuild_logger_tofree
which is handled differently and so must be retained.  It seems better
to keep the xch code internally consistent, and decoupled from the
general default.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
v2: New in this version of the series
---
 tools/libs/call/core.c             | 10 ----------
 tools/libs/call/private.h          |  2 +-
 tools/libs/devicemodel/core.c      | 11 -----------
 tools/libs/devicemodel/private.h   |  2 +-
 tools/libs/evtchn/core.c           | 10 ----------
 tools/libs/evtchn/private.h        |  2 +-
 tools/libs/foreignmemory/core.c    | 10 ----------
 tools/libs/foreignmemory/private.h |  2 +-
 tools/libs/gnttab/gntshr_core.c    | 10 ----------
 tools/libs/gnttab/gnttab_core.c    | 10 ----------
 tools/libs/gnttab/private.h        |  2 +-
 11 files changed, 5 insertions(+), 66 deletions(-)

diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index 57d3a33e6b..ee9be948e7 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -54,14 +54,6 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
     xcall->buffer_cache_misses = 0;
     xcall->buffer_cache_toobig = 0;
     xcall->logger = logger;
-    xcall->logger_tofree = NULL;
-
-    if (!xcall->logger) {
-        xcall->logger = xcall->logger_tofree =
-            (xentoollog_logger*)
-            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
-        if (!xcall->logger) goto err;
-    }
 
     rc = osdep_xencall_open(xcall);
     if ( rc  < 0 ) goto err;
@@ -71,7 +63,6 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
 err:
     xentoolcore__deregister_active_handle(&xcall->tc_ah);
     osdep_xencall_close(xcall);
-    xtl_logger_destroy(xcall->logger_tofree);
     free(xcall);
     return NULL;
 }
@@ -86,7 +77,6 @@ int xencall_close(xencall_handle *xcall)
     xentoolcore__deregister_active_handle(&xcall->tc_ah);
     rc = osdep_xencall_close(xcall);
     buffer_release_cache(xcall);
-    xtl_logger_destroy(xcall->logger_tofree);
     free(xcall);
     return rc;
 }
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 21f992b37e..a2d00b2b6a 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -18,7 +18,7 @@
 #endif
 
 struct xencall_handle {
-    xentoollog_logger *logger, *logger_tofree;
+    xentoollog_logger *logger;
     unsigned flags;
 
                      /* partially     with /dev/     no /dev/      */
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index f76e3d305e..b91f6b4ee9 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -46,15 +46,6 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
 
     dmod->flags = open_flags;
     dmod->logger = logger;
-    dmod->logger_tofree = NULL;
-
-    if (!dmod->logger) {
-        dmod->logger = dmod->logger_tofree =
-            (xentoollog_logger*)
-            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
-        if (!dmod->logger)
-            goto err;
-    }
 
     dmod->xcall = xencall_open(dmod->logger, 0);
     if (!dmod->xcall)
@@ -67,7 +58,6 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
     return dmod;
 
 err:
-    xtl_logger_destroy(dmod->logger_tofree);
     xentoolcore__deregister_active_handle(&dmod->tc_ah);
     xencall_close(dmod->xcall);
     free(dmod);
@@ -85,7 +75,6 @@ int xendevicemodel_close(xendevicemodel_handle *dmod)
 
     xentoolcore__deregister_active_handle(&dmod->tc_ah);
     xencall_close(dmod->xcall);
-    xtl_logger_destroy(dmod->logger_tofree);
     free(dmod);
     return rc;
 }
diff --git a/tools/libs/devicemodel/private.h b/tools/libs/devicemodel/private.h
index c4a225f8af..edee969313 100644
--- a/tools/libs/devicemodel/private.h
+++ b/tools/libs/devicemodel/private.h
@@ -10,7 +10,7 @@
 #include <xentoolcore_internal.h>
 
 struct xendevicemodel_handle {
-    xentoollog_logger *logger, *logger_tofree;
+    xentoollog_logger *logger;
     unsigned int flags;
     xencall_handle *xcall;
     int fd;
diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c
index aff6ecfaa0..d1e53284e0 100644
--- a/tools/libs/evtchn/core.c
+++ b/tools/libs/evtchn/core.c
@@ -37,18 +37,10 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags)
 
     xce->fd = -1;
     xce->logger = logger;
-    xce->logger_tofree  = NULL;
 
     xce->tc_ah.restrict_callback = all_restrict_cb;
     xentoolcore__register_active_handle(&xce->tc_ah);
 
-    if (!xce->logger) {
-        xce->logger = xce->logger_tofree =
-            (xentoollog_logger*)
-            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
-        if (!xce->logger) goto err;
-    }
-
     rc = osdep_evtchn_open(xce);
     if ( rc  < 0 ) goto err;
 
@@ -57,7 +49,6 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags)
 err:
     xentoolcore__deregister_active_handle(&xce->tc_ah);
     osdep_evtchn_close(xce);
-    xtl_logger_destroy(xce->logger_tofree);
     free(xce);
     return NULL;
 }
@@ -71,7 +62,6 @@ int xenevtchn_close(xenevtchn_handle *xce)
 
     xentoolcore__deregister_active_handle(&xce->tc_ah);
     rc = osdep_evtchn_close(xce);
-    xtl_logger_destroy(xce->logger_tofree);
     free(xce);
     return rc;
 }
diff --git a/tools/libs/evtchn/private.h b/tools/libs/evtchn/private.h
index 31e595bea2..a272895fe5 100644
--- a/tools/libs/evtchn/private.h
+++ b/tools/libs/evtchn/private.h
@@ -9,7 +9,7 @@
 #include <xen/xen.h>
 
 struct xenevtchn_handle {
-    xentoollog_logger *logger, *logger_tofree;
+    xentoollog_logger *logger;
     int fd;
     Xentoolcore__Active_Handle tc_ah;
 };
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 63f12e2450..d485dd8672 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -41,18 +41,10 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
 
     fmem->fd = -1;
     fmem->logger = logger;
-    fmem->logger_tofree = NULL;
 
     fmem->tc_ah.restrict_callback = all_restrict_cb;
     xentoolcore__register_active_handle(&fmem->tc_ah);
 
-    if (!fmem->logger) {
-        fmem->logger = fmem->logger_tofree =
-            (xentoollog_logger*)
-            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
-        if (!fmem->logger) goto err;
-    }
-
     rc = osdep_xenforeignmemory_open(fmem);
     if ( rc  < 0 ) goto err;
 
@@ -61,7 +53,6 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
 err:
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
     osdep_xenforeignmemory_close(fmem);
-    xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return NULL;
 }
@@ -75,7 +66,6 @@ int xenforeignmemory_close(xenforeignmemory_handle *fmem)
 
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
     rc = osdep_xenforeignmemory_close(fmem);
-    xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return rc;
 }
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 8f1bf081ed..9030de9740 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -19,7 +19,7 @@
 #endif
 
 struct xenforeignmemory_handle {
-    xentoollog_logger *logger, *logger_tofree;
+    xentoollog_logger *logger;
     unsigned flags;
     int fd;
     Xentoolcore__Active_Handle tc_ah;
diff --git a/tools/libs/gnttab/gntshr_core.c b/tools/libs/gnttab/gntshr_core.c
index 1117e29c91..38cf364897 100644
--- a/tools/libs/gnttab/gntshr_core.c
+++ b/tools/libs/gnttab/gntshr_core.c
@@ -31,14 +31,6 @@ xengntshr_handle *xengntshr_open(xentoollog_logger *logger, unsigned open_flags)
 
     xgs->fd = -1;
     xgs->logger = logger;
-    xgs->logger_tofree  = NULL;
-
-    if (!xgs->logger) {
-        xgs->logger = xgs->logger_tofree =
-            (xentoollog_logger*)
-            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
-        if (!xgs->logger) goto err;
-    }
 
     rc = osdep_gntshr_open(xgs);
     if ( rc  < 0 ) goto err;
@@ -47,7 +39,6 @@ xengntshr_handle *xengntshr_open(xentoollog_logger *logger, unsigned open_flags)
 
 err:
     osdep_gntshr_close(xgs);
-    xtl_logger_destroy(xgs->logger_tofree);
     free(xgs);
     return NULL;
 }
@@ -60,7 +51,6 @@ int xengntshr_close(xengntshr_handle *xgs)
         return 0;
 
     rc = osdep_gntshr_close(xgs);
-    xtl_logger_destroy(xgs->logger_tofree);
     free(xgs);
     return rc;
 }
diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
index 92e7228a26..a67f444245 100644
--- a/tools/libs/gnttab/gnttab_core.c
+++ b/tools/libs/gnttab/gnttab_core.c
@@ -37,18 +37,10 @@ xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
 
     xgt->fd = -1;
     xgt->logger = logger;
-    xgt->logger_tofree  = NULL;
 
     xgt->tc_ah.restrict_callback = all_restrict_cb;
     xentoolcore__register_active_handle(&xgt->tc_ah);
 
-    if (!xgt->logger) {
-        xgt->logger = xgt->logger_tofree =
-            (xentoollog_logger*)
-            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
-        if (!xgt->logger) goto err;
-    }
-
     rc = osdep_gnttab_open(xgt);
     if ( rc  < 0 ) goto err;
 
@@ -57,7 +49,6 @@ xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
 err:
     xentoolcore__deregister_active_handle(&xgt->tc_ah);
     osdep_gnttab_close(xgt);
-    xtl_logger_destroy(xgt->logger_tofree);
     free(xgt);
     return NULL;
 }
@@ -71,7 +62,6 @@ int xengnttab_close(xengnttab_handle *xgt)
 
     xentoolcore__deregister_active_handle(&xgt->tc_ah);
     rc = osdep_gnttab_close(xgt);
-    xtl_logger_destroy(xgt->logger_tofree);
     free(xgt);
     return rc;
 }
diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
index c5e23639b1..cdb155761e 100644
--- a/tools/libs/gnttab/private.h
+++ b/tools/libs/gnttab/private.h
@@ -12,7 +12,7 @@
 #define GSERROR(_l, _f...) xtl_log(_l, XTL_ERROR, errno, "gntshr", _f)
 
 struct xengntdev_handle {
-    xentoollog_logger *logger, *logger_tofree;
+    xentoollog_logger *logger;
     int fd;
     Xentoolcore__Active_Handle tc_ah;
 };
-- 
2.11.0


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

Re: [PATCH v2 03/11] tools/libs/*: Rely on the default logger

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 12290 bytes --]

On Thu, Nov 08, 2018 at 05:07:57PM +0000, Ian Jackson wrote:
> Delete 11 entirely formulaic conditional calls to
>   xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> and associated logger_tofree variables, error handling, etc.
> 
> No overall functional change, although some memory allocation errors
> may no longer occur.
> 
> After this there are still several calls to
> xtl_createlogger_stdiostream in tree, but they almost all have
> non-default message level etc. so it is not obvious that they should
> be replaced.
> 
> The exception is in xc_private.c where xch->error_handler is
> initialised using a copy of the default initialisation boilerplate
> (ant there is the associated xch->error_handler_tofree).  However,
> there is also xch->dombuild_logger, and xch->dombuild_logger_tofree
> which is handled differently and so must be retained.  It seems better
> to keep the xch code internally consistent, and decoupled from the
> general default.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
> v2: New in this version of the series
> ---
>  tools/libs/call/core.c             | 10 ----------
>  tools/libs/call/private.h          |  2 +-
>  tools/libs/devicemodel/core.c      | 11 -----------
>  tools/libs/devicemodel/private.h   |  2 +-
>  tools/libs/evtchn/core.c           | 10 ----------
>  tools/libs/evtchn/private.h        |  2 +-
>  tools/libs/foreignmemory/core.c    | 10 ----------
>  tools/libs/foreignmemory/private.h |  2 +-
>  tools/libs/gnttab/gntshr_core.c    | 10 ----------
>  tools/libs/gnttab/gnttab_core.c    | 10 ----------
>  tools/libs/gnttab/private.h        |  2 +-
>  11 files changed, 5 insertions(+), 66 deletions(-)
> 
> diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
> index 57d3a33e6b..ee9be948e7 100644
> --- a/tools/libs/call/core.c
> +++ b/tools/libs/call/core.c
> @@ -54,14 +54,6 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
>      xcall->buffer_cache_misses = 0;
>      xcall->buffer_cache_toobig = 0;
>      xcall->logger = logger;
> -    xcall->logger_tofree = NULL;
> -
> -    if (!xcall->logger) {
> -        xcall->logger = xcall->logger_tofree =
> -            (xentoollog_logger*)
> -            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> -        if (!xcall->logger) goto err;
> -    }
>  
>      rc = osdep_xencall_open(xcall);
>      if ( rc  < 0 ) goto err;
> @@ -71,7 +63,6 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
>  err:
>      xentoolcore__deregister_active_handle(&xcall->tc_ah);
>      osdep_xencall_close(xcall);
> -    xtl_logger_destroy(xcall->logger_tofree);
>      free(xcall);
>      return NULL;
>  }
> @@ -86,7 +77,6 @@ int xencall_close(xencall_handle *xcall)
>      xentoolcore__deregister_active_handle(&xcall->tc_ah);
>      rc = osdep_xencall_close(xcall);
>      buffer_release_cache(xcall);
> -    xtl_logger_destroy(xcall->logger_tofree);
>      free(xcall);
>      return rc;
>  }
> diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
> index 21f992b37e..a2d00b2b6a 100644
> --- a/tools/libs/call/private.h
> +++ b/tools/libs/call/private.h
> @@ -18,7 +18,7 @@
>  #endif
>  
>  struct xencall_handle {
> -    xentoollog_logger *logger, *logger_tofree;
> +    xentoollog_logger *logger;
>      unsigned flags;
>  
>                       /* partially     with /dev/     no /dev/      */
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index f76e3d305e..b91f6b4ee9 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -46,15 +46,6 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
>  
>      dmod->flags = open_flags;
>      dmod->logger = logger;
> -    dmod->logger_tofree = NULL;
> -
> -    if (!dmod->logger) {
> -        dmod->logger = dmod->logger_tofree =
> -            (xentoollog_logger*)
> -            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> -        if (!dmod->logger)
> -            goto err;
> -    }
>  
>      dmod->xcall = xencall_open(dmod->logger, 0);
>      if (!dmod->xcall)
> @@ -67,7 +58,6 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
>      return dmod;
>  
>  err:
> -    xtl_logger_destroy(dmod->logger_tofree);
>      xentoolcore__deregister_active_handle(&dmod->tc_ah);
>      xencall_close(dmod->xcall);
>      free(dmod);
> @@ -85,7 +75,6 @@ int xendevicemodel_close(xendevicemodel_handle *dmod)
>  
>      xentoolcore__deregister_active_handle(&dmod->tc_ah);
>      xencall_close(dmod->xcall);
> -    xtl_logger_destroy(dmod->logger_tofree);
>      free(dmod);
>      return rc;
>  }
> diff --git a/tools/libs/devicemodel/private.h b/tools/libs/devicemodel/private.h
> index c4a225f8af..edee969313 100644
> --- a/tools/libs/devicemodel/private.h
> +++ b/tools/libs/devicemodel/private.h
> @@ -10,7 +10,7 @@
>  #include <xentoolcore_internal.h>
>  
>  struct xendevicemodel_handle {
> -    xentoollog_logger *logger, *logger_tofree;
> +    xentoollog_logger *logger;
>      unsigned int flags;
>      xencall_handle *xcall;
>      int fd;
> diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c
> index aff6ecfaa0..d1e53284e0 100644
> --- a/tools/libs/evtchn/core.c
> +++ b/tools/libs/evtchn/core.c
> @@ -37,18 +37,10 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags)
>  
>      xce->fd = -1;
>      xce->logger = logger;
> -    xce->logger_tofree  = NULL;
>  
>      xce->tc_ah.restrict_callback = all_restrict_cb;
>      xentoolcore__register_active_handle(&xce->tc_ah);
>  
> -    if (!xce->logger) {
> -        xce->logger = xce->logger_tofree =
> -            (xentoollog_logger*)
> -            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> -        if (!xce->logger) goto err;
> -    }
> -
>      rc = osdep_evtchn_open(xce);
>      if ( rc  < 0 ) goto err;
>  
> @@ -57,7 +49,6 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags)
>  err:
>      xentoolcore__deregister_active_handle(&xce->tc_ah);
>      osdep_evtchn_close(xce);
> -    xtl_logger_destroy(xce->logger_tofree);
>      free(xce);
>      return NULL;
>  }
> @@ -71,7 +62,6 @@ int xenevtchn_close(xenevtchn_handle *xce)
>  
>      xentoolcore__deregister_active_handle(&xce->tc_ah);
>      rc = osdep_evtchn_close(xce);
> -    xtl_logger_destroy(xce->logger_tofree);
>      free(xce);
>      return rc;
>  }
> diff --git a/tools/libs/evtchn/private.h b/tools/libs/evtchn/private.h
> index 31e595bea2..a272895fe5 100644
> --- a/tools/libs/evtchn/private.h
> +++ b/tools/libs/evtchn/private.h
> @@ -9,7 +9,7 @@
>  #include <xen/xen.h>
>  
>  struct xenevtchn_handle {
> -    xentoollog_logger *logger, *logger_tofree;
> +    xentoollog_logger *logger;
>      int fd;
>      Xentoolcore__Active_Handle tc_ah;
>  };
> diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
> index 63f12e2450..d485dd8672 100644
> --- a/tools/libs/foreignmemory/core.c
> +++ b/tools/libs/foreignmemory/core.c
> @@ -41,18 +41,10 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
>  
>      fmem->fd = -1;
>      fmem->logger = logger;
> -    fmem->logger_tofree = NULL;
>  
>      fmem->tc_ah.restrict_callback = all_restrict_cb;
>      xentoolcore__register_active_handle(&fmem->tc_ah);
>  
> -    if (!fmem->logger) {
> -        fmem->logger = fmem->logger_tofree =
> -            (xentoollog_logger*)
> -            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> -        if (!fmem->logger) goto err;
> -    }
> -
>      rc = osdep_xenforeignmemory_open(fmem);
>      if ( rc  < 0 ) goto err;
>  
> @@ -61,7 +53,6 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
>  err:
>      xentoolcore__deregister_active_handle(&fmem->tc_ah);
>      osdep_xenforeignmemory_close(fmem);
> -    xtl_logger_destroy(fmem->logger_tofree);
>      free(fmem);
>      return NULL;
>  }
> @@ -75,7 +66,6 @@ int xenforeignmemory_close(xenforeignmemory_handle *fmem)
>  
>      xentoolcore__deregister_active_handle(&fmem->tc_ah);
>      rc = osdep_xenforeignmemory_close(fmem);
> -    xtl_logger_destroy(fmem->logger_tofree);
>      free(fmem);
>      return rc;
>  }
> diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
> index 8f1bf081ed..9030de9740 100644
> --- a/tools/libs/foreignmemory/private.h
> +++ b/tools/libs/foreignmemory/private.h
> @@ -19,7 +19,7 @@
>  #endif
>  
>  struct xenforeignmemory_handle {
> -    xentoollog_logger *logger, *logger_tofree;
> +    xentoollog_logger *logger;
>      unsigned flags;
>      int fd;
>      Xentoolcore__Active_Handle tc_ah;
> diff --git a/tools/libs/gnttab/gntshr_core.c b/tools/libs/gnttab/gntshr_core.c
> index 1117e29c91..38cf364897 100644
> --- a/tools/libs/gnttab/gntshr_core.c
> +++ b/tools/libs/gnttab/gntshr_core.c
> @@ -31,14 +31,6 @@ xengntshr_handle *xengntshr_open(xentoollog_logger *logger, unsigned open_flags)
>  
>      xgs->fd = -1;
>      xgs->logger = logger;
> -    xgs->logger_tofree  = NULL;
> -
> -    if (!xgs->logger) {
> -        xgs->logger = xgs->logger_tofree =
> -            (xentoollog_logger*)
> -            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> -        if (!xgs->logger) goto err;
> -    }
>  
>      rc = osdep_gntshr_open(xgs);
>      if ( rc  < 0 ) goto err;
> @@ -47,7 +39,6 @@ xengntshr_handle *xengntshr_open(xentoollog_logger *logger, unsigned open_flags)
>  
>  err:
>      osdep_gntshr_close(xgs);
> -    xtl_logger_destroy(xgs->logger_tofree);
>      free(xgs);
>      return NULL;
>  }
> @@ -60,7 +51,6 @@ int xengntshr_close(xengntshr_handle *xgs)
>          return 0;
>  
>      rc = osdep_gntshr_close(xgs);
> -    xtl_logger_destroy(xgs->logger_tofree);
>      free(xgs);
>      return rc;
>  }
> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
> index 92e7228a26..a67f444245 100644
> --- a/tools/libs/gnttab/gnttab_core.c
> +++ b/tools/libs/gnttab/gnttab_core.c
> @@ -37,18 +37,10 @@ xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
>  
>      xgt->fd = -1;
>      xgt->logger = logger;
> -    xgt->logger_tofree  = NULL;
>  
>      xgt->tc_ah.restrict_callback = all_restrict_cb;
>      xentoolcore__register_active_handle(&xgt->tc_ah);
>  
> -    if (!xgt->logger) {
> -        xgt->logger = xgt->logger_tofree =
> -            (xentoollog_logger*)
> -            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> -        if (!xgt->logger) goto err;
> -    }
> -
>      rc = osdep_gnttab_open(xgt);
>      if ( rc  < 0 ) goto err;
>  
> @@ -57,7 +49,6 @@ xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
>  err:
>      xentoolcore__deregister_active_handle(&xgt->tc_ah);
>      osdep_gnttab_close(xgt);
> -    xtl_logger_destroy(xgt->logger_tofree);
>      free(xgt);
>      return NULL;
>  }
> @@ -71,7 +62,6 @@ int xengnttab_close(xengnttab_handle *xgt)
>  
>      xentoolcore__deregister_active_handle(&xgt->tc_ah);
>      rc = osdep_gnttab_close(xgt);
> -    xtl_logger_destroy(xgt->logger_tofree);
>      free(xgt);
>      return rc;
>  }
> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> index c5e23639b1..cdb155761e 100644
> --- a/tools/libs/gnttab/private.h
> +++ b/tools/libs/gnttab/private.h
> @@ -12,7 +12,7 @@
>  #define GSERROR(_l, _f...) xtl_log(_l, XTL_ERROR, errno, "gntshr", _f)
>  
>  struct xengntdev_handle {
> -    xentoollog_logger *logger, *logger_tofree;
> +    xentoollog_logger *logger;
>      int fd;
>      Xentoolcore__Active_Handle tc_ah;
>  };

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2 03/11] tools/libs/*: Rely on the default logger

[Wei Liu]

On Thu, Nov 08, 2018 at 05:07:57PM +0000, Ian Jackson wrote:
> Delete 11 entirely formulaic conditional calls to
>   xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
> and associated logger_tofree variables, error handling, etc.
> 
> No overall functional change, although some memory allocation errors
> may no longer occur.
> 
> After this there are still several calls to
> xtl_createlogger_stdiostream in tree, but they almost all have
> non-default message level etc. so it is not obvious that they should
> be replaced.
> 
> The exception is in xc_private.c where xch->error_handler is
> initialised using a copy of the default initialisation boilerplate
> (ant there is the associated xch->error_handler_tofree).  However,
> there is also xch->dombuild_logger, and xch->dombuild_logger_tofree
> which is handled differently and so must be retained.  It seems better
> to keep the xch code internally consistent, and decoupled from the
> general default.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v2 04/11] tools/libvchan: Initialise xs_transaction_t to XBT_NULL, not NULL

[Ian Jackson]

This is an integer type, not a pointer.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libvchan/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index ba5a6eb29e..180833dc2f 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -250,7 +250,7 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 	char buf[64];
 	char ref[16];
 	char* domid_str = NULL;
-	xs_transaction_t xs_trans = NULL;
+	xs_transaction_t xs_trans = XBT_NULL;
 	xs = xs_domain_open();
 	if (!xs)
 		goto fail;
-- 
2.11.0


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

[PATCH v2 05/11] tools/xenstore: Document that xs_close(0) is OK.

[Ian Jackson]

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/include/xenstore.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstore/include/xenstore.h b/tools/xenstore/include/xenstore.h
index 064b62c455..889dc23863 100644
--- a/tools/xenstore/include/xenstore.h
+++ b/tools/xenstore/include/xenstore.h
@@ -77,7 +77,7 @@ typedef uint32_t xs_transaction_t;
 struct xs_handle *xs_open(unsigned long flags);
 
 /* Close the connection to the xs daemon. */
-void xs_close(struct xs_handle *xsh);
+void xs_close(struct xs_handle *xsh /* NULL ok */);
 
 /* Connect to the xs daemon.
  * Returns a handle or NULL.
-- 
2.11.0


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

[PATCH v2 06/11] tools/libvchan: init_xs_srv: Simplify error handling (1)

[Ian Jackson]

* Use xs_close instead of the deprecated xs_daemon_close.

* Initialise xs to NULL.    That means xs_close can now be called in
  all cases.  Move it to the fail clause.

* free(domid_str) is already safe in all cases since domid_str is
  initialised to NULL.  Move it to the fail clause.

No overall functional change: xs_close is the same as xs_daemon_close;
and it and free are now sometimes called on NULL, but those are no-ops.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libvchan/init.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 180833dc2f..9c61c720d1 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -245,7 +245,7 @@ fail:
 static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base, int ring_ref)
 {
 	int ret = -1;
-	struct xs_handle *xs;
+	struct xs_handle *xs = NULL;
 	struct xs_permissions perms[2];
 	char buf[64];
 	char ref[16];
@@ -292,9 +292,9 @@ retry_transaction:
 		ret = 0;
 	}
  fail_xs_open:
-	free(domid_str);
-	xs_daemon_close(xs);
  fail:
+	free(domid_str);
+	xs_close(xs);
 	return ret;
 }
 
-- 
2.11.0


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

[PATCH v2 07/11] tools/libvchan: init_xs_srv: Simplify error handling (2)

[Ian Jackson]

* Abolish fail_xs_open which is now exactly the same as fail.

* Change all gotos to refer to fail instead.

No functional change.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libvchan/init.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 9c61c720d1..f099765a38 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -256,7 +256,7 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 		goto fail;
 	domid_str = xs_read(xs, 0, "domid", NULL);
 	if (!domid_str)
-		goto fail_xs_open;
+		goto fail;
 
 	// owner domain is us
 	perms[0].id = atoi(domid_str);
@@ -269,21 +269,21 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 retry_transaction:
 	xs_trans = xs_transaction_start(xs);
 	if (!xs_trans)
-		goto fail_xs_open;
+		goto fail;
 
 	snprintf(ref, sizeof ref, "%d", ring_ref);
 	snprintf(buf, sizeof buf, "%s/ring-ref", xs_base);
 	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail_xs_open;
+		goto fail;
 	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail_xs_open;
+		goto fail;
 
 	snprintf(ref, sizeof ref, "%d", ctrl->event_port);
 	snprintf(buf, sizeof buf, "%s/event-channel", xs_base);
 	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail_xs_open;
+		goto fail;
 	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail_xs_open;
+		goto fail;
 
 	if (!xs_transaction_end(xs, xs_trans, 0)) {
 		if (errno == EAGAIN)
@@ -291,7 +291,6 @@ retry_transaction:
 	} else {
 		ret = 0;
 	}
- fail_xs_open:
  fail:
 	free(domid_str);
 	xs_close(xs);
-- 
2.11.0


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

[PATCH v2 08/11] tools/libvchan: init_xs_srv: Turn xs retry from goto into for (; ; )

[Ian Jackson]

No functional change.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libvchan/init.c | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index f099765a38..d987acd338 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -266,31 +266,33 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 	perms[1].id = domain;
 	perms[1].perms = XS_PERM_READ;
 
-retry_transaction:
-	xs_trans = xs_transaction_start(xs);
-	if (!xs_trans)
-		goto fail;
-
-	snprintf(ref, sizeof ref, "%d", ring_ref);
-	snprintf(buf, sizeof buf, "%s/ring-ref", xs_base);
-	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail;
-	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail;
-
-	snprintf(ref, sizeof ref, "%d", ctrl->event_port);
-	snprintf(buf, sizeof buf, "%s/event-channel", xs_base);
-	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail;
-	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail;
-
-	if (!xs_transaction_end(xs, xs_trans, 0)) {
-		if (errno == EAGAIN)
-			goto retry_transaction;
-	} else {
-		ret = 0;
+	for (;;) {
+		xs_trans = xs_transaction_start(xs);
+		if (!xs_trans)
+			goto fail;
+
+		snprintf(ref, sizeof ref, "%d", ring_ref);
+		snprintf(buf, sizeof buf, "%s/ring-ref", xs_base);
+		if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+			goto fail;
+		if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+			goto fail;
+
+		snprintf(ref, sizeof ref, "%d", ctrl->event_port);
+		snprintf(buf, sizeof buf, "%s/event-channel", xs_base);
+		if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+			goto fail;
+		if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+			goto fail;
+
+		if (xs_transaction_end(xs, xs_trans, 0))
+			break;
+		else if (errno != EAGAIN)
+			goto fail;
+		/* EAGAIN, retry */
 	}
+	ret = 0;
+
  fail:
 	free(domid_str);
 	xs_close(xs);
-- 
2.11.0


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

[PATCH v2 09/11] tools/libvchan: Add xentoollog to direct dependencies

[Ian Jackson]

We are going to add a call to xtl_log.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libvchan/Makefile       | 6 +++---
 tools/libvchan/xenvchan.pc.in | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libvchan/Makefile b/tools/libvchan/Makefile
index de9b44978f..c236a0f9e6 100644
--- a/tools/libvchan/Makefile
+++ b/tools/libvchan/Makefile
@@ -10,9 +10,9 @@ NODE_OBJS = node.o
 NODE2_OBJS = node-select.o
 
 LIBVCHAN_PIC_OBJS = $(patsubst %.o,%.opic,$(LIBVCHAN_OBJS))
-LIBVCHAN_LIBS = $(LDLIBS_libxenstore) $(LDLIBS_libxengnttab) $(LDLIBS_libxenevtchn)
-$(LIBVCHAN_OBJS) $(LIBVCHAN_PIC_OBJS): CFLAGS += $(CFLAGS_libxenstore) $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
-$(NODE_OBJS) $(NODE2_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
+LIBVCHAN_LIBS = $(LDLIBS_libxenstore) $(LDLIBS_libxengnttab) $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog)
+$(LIBVCHAN_OBJS) $(LIBVCHAN_PIC_OBJS): CFLAGS += $(CFLAGS_libxenstore) $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn) $(CFLAGS_libxentoollog)
+$(NODE_OBJS) $(NODE2_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn) $(CFLAGS_libxentoollog)
 
 MAJOR = 4.12
 MINOR = 0
diff --git a/tools/libvchan/xenvchan.pc.in b/tools/libvchan/xenvchan.pc.in
index 6fd13108d2..4b055c6c8f 100644
--- a/tools/libvchan/xenvchan.pc.in
+++ b/tools/libvchan/xenvchan.pc.in
@@ -7,4 +7,4 @@ Description: The Xenvchan library for Xen hypervisor
 Version: @@version@@
 Cflags: -I${includedir} @@cflagslocal@@
 Libs: @@libsflag@@${libdir} -lxenvchan
-Requires.private: xentoollog,xenstore,xenevtchn,xengnttab
+Requires.private: xentoollog,xenstore,xenevtchn,xengnttab,xentoollog
-- 
2.11.0


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

[PATCH v2 10/11] tools/libvchan: libxenvchan_*_init: Promise an errno

[Ian Jackson]

These functions do in fact leave errno set.  We are going to want to
use this.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libvchan/libxenvchan.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libvchan/libxenvchan.h b/tools/libvchan/libxenvchan.h
index d6010b145d..e4ccca1ff0 100644
--- a/tools/libvchan/libxenvchan.h
+++ b/tools/libvchan/libxenvchan.h
@@ -95,7 +95,7 @@ struct libxenvchan {
  * @param xs_path Base xenstore path for storing ring/event data
  * @param send_min The minimum size (in bytes) of the send ring (left)
  * @param recv_min The minimum size (in bytes) of the receive ring (right)
- * @return The structure, or NULL in case of an error
+ * @return The structure, or NULL in case of an error (setting errno)
  */
 struct libxenvchan *libxenvchan_server_init(struct xentoollog_logger *logger,
                                             int domain, const char* xs_path,
@@ -108,7 +108,7 @@ struct libxenvchan *libxenvchan_server_init(struct xentoollog_logger *logger,
  * @param logger Logger for libxc errors
  * @param domain The peer domain to connect to
  * @param xs_path Base xenstore path for storing ring/event data
- * @return The structure, or NULL in case of an error
+ * @return The structure, or NULL in case of an error (setting errno)
  */
 struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
                                             int domain, const char* xs_path);
-- 
2.11.0


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

[PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Ian Jackson]

* Promise that we will set errno to ENOENT if the server is not
  yet set up.
* Arrange that all ENOENT returns other than from the read of ring-ref
  are turned into EIO, logging when we do so.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libvchan/init.c        | 11 ++++++++++-
 tools/libvchan/libxenvchan.h |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index d987acd338..e58f6bf9ac 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -45,6 +45,7 @@
 #include <xen/sys/gntalloc.h>
 #include <xen/sys/gntdev.h>
 #include <libxenvchan.h>
+#include <xentoollog.h>
 
 #ifndef PAGE_SHIFT
 #define PAGE_SHIFT 12
@@ -419,7 +420,7 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
 	snprintf(buf, sizeof buf, "%s/ring-ref", xs_path);
 	ref = xs_read(xs, 0, buf, &len);
 	if (!ref)
-		goto fail;
+		goto fail_allow_enoent;
 	ring_ref = atoi(ref);
 	free(ref);
 	if (!ring_ref)
@@ -452,7 +453,15 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
 	if (xs)
 		xs_daemon_close(xs);
 	return ctrl;
+
  fail:
+	if (errno == ENOENT) {
+		xtl_log(logger, XTL_ERROR, errno, "vchan",
+			"error talking to server `%s', returning EIO",
+			xs_path);
+		errno = EIO;
+	}
+ fail_allow_enoent:
 	libxenvchan_close(ctrl);
 	ctrl = NULL;
 	goto out;
diff --git a/tools/libvchan/libxenvchan.h b/tools/libvchan/libxenvchan.h
index e4ccca1ff0..8a4ec2ce4c 100644
--- a/tools/libvchan/libxenvchan.h
+++ b/tools/libvchan/libxenvchan.h
@@ -105,6 +105,10 @@ struct libxenvchan *libxenvchan_server_init(struct xentoollog_logger *logger,
  * safely, however no locking is performed, so you must prevent multiple clients
  * from connecting to a single server.
  *
+ * Failing with ENOENT means the server has not yet called
+ * libxenvchan_server_init, You may wait for a server to appear by
+ * setting a xenstore watch on xs_path.
+ *
  * @param logger Logger for libxc errors
  * @param domain The peer domain to connect to
  * @param xs_path Base xenstore path for storing ring/event data
-- 
2.11.0


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

Re: [PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2778 bytes --]

On Thu, Nov 08, 2018 at 05:08:05PM +0000, Ian Jackson wrote:
> * Promise that we will set errno to ENOENT if the server is not
>   yet set up.
> * Arrange that all ENOENT returns other than from the read of ring-ref
>   are turned into EIO, logging when we do so.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Since xtl_log now accepts NULL, it should be fine. Thanks!
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
>  tools/libvchan/init.c        | 11 ++++++++++-
>  tools/libvchan/libxenvchan.h |  4 ++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
> index d987acd338..e58f6bf9ac 100644
> --- a/tools/libvchan/init.c
> +++ b/tools/libvchan/init.c
> @@ -45,6 +45,7 @@
>  #include <xen/sys/gntalloc.h>
>  #include <xen/sys/gntdev.h>
>  #include <libxenvchan.h>
> +#include <xentoollog.h>
>  
>  #ifndef PAGE_SHIFT
>  #define PAGE_SHIFT 12
> @@ -419,7 +420,7 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
>  	snprintf(buf, sizeof buf, "%s/ring-ref", xs_path);
>  	ref = xs_read(xs, 0, buf, &len);
>  	if (!ref)
> -		goto fail;
> +		goto fail_allow_enoent;
>  	ring_ref = atoi(ref);
>  	free(ref);
>  	if (!ring_ref)
> @@ -452,7 +453,15 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
>  	if (xs)
>  		xs_daemon_close(xs);
>  	return ctrl;
> +
>   fail:
> +	if (errno == ENOENT) {
> +		xtl_log(logger, XTL_ERROR, errno, "vchan",
> +			"error talking to server `%s', returning EIO",
> +			xs_path);
> +		errno = EIO;
> +	}
> + fail_allow_enoent:
>  	libxenvchan_close(ctrl);
>  	ctrl = NULL;
>  	goto out;
> diff --git a/tools/libvchan/libxenvchan.h b/tools/libvchan/libxenvchan.h
> index e4ccca1ff0..8a4ec2ce4c 100644
> --- a/tools/libvchan/libxenvchan.h
> +++ b/tools/libvchan/libxenvchan.h
> @@ -105,6 +105,10 @@ struct libxenvchan *libxenvchan_server_init(struct xentoollog_logger *logger,
>   * safely, however no locking is performed, so you must prevent multiple clients
>   * from connecting to a single server.
>   *
> + * Failing with ENOENT means the server has not yet called
> + * libxenvchan_server_init, You may wait for a server to appear by
> + * setting a xenstore watch on xs_path.
> + *
>   * @param logger Logger for libxc errors
>   * @param domain The peer domain to connect to
>   * @param xs_path Base xenstore path for storing ring/event data

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [Xen-devel] [PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server"):
> Since xtl_log now accepts NULL, it should be fine. Thanks!
> Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Thanks.

I am reluctant to commit this series without having been able to test
it.  There's the libvchan node.c and node-select.c but they seem a bit
raw.

Do you have a script or something to wrap them up ?

Ideally I guess this would be run by osstest somehow.

Ian.

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

Re: [PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2360 bytes --]

On Mon, Nov 12, 2018 at 01:52:49PM +0000, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("Re: [Xen-devel] [PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server"):
> > Since xtl_log now accepts NULL, it should be fine. Thanks!
> > Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Thanks.
> 
> I am reluctant to commit this series without having been able to test
> it.  There's the libvchan node.c and node-select.c but they seem a bit
> raw.
> 
> Do you have a script or something to wrap them up ?

Not really. I've done a manual test, in two domains:

server:    vchan-node2 server $xid2 /local/domain/$xid1/data/vchan/$xid2/1234

client:    vchan-node2 client $xid1 /local/domain/$xid1/data/vchan/$xid2/1234

($xid1 and $xid2 are domains ID of server and client respectively)

When start it in this order, it works fine. But if I start the client
earlier, I get libxenvchan_*_init: Permission denied. That makes sense -
before server setup permissions on xenstore, the client can't read the
entries but not because of ENOENT, but EACCESS.

> Ideally I guess this would be run by osstest somehow.

I've tried... But stumbled across a bunch of problems:
1. Xen tools are not installed in guest
2. vchan-node* are not installed at all
3. I need two domUs for this (or maybe dom0 and domU?) and processes
running in parallel (this is where my perl ends).

I gave up after point 2. Do you want patches for point 1 and 2 anyway?
New ts-guest-xen-install assume the build for dom0 is fine for domU too
(which is true for Debian)...
As for the second point, I've installed it into $(bindir) as a quick
hack, but not sure if this is the best option.
I'm not jet sure how to put all those pieces together. I guess I need to
edit sg-run-job, but that's jet another language...

BTW vchan doesn't like loopback connections (within the same domain), as
gnttab doesn't like it. Is it something fixable, or is it working as
designed? If the latter, I think at least kernel shouldn't panic in that
case... More details:
https://github.com/QubesOS/qubes-issues/issues/951

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Wei Liu]

On Thu, Nov 08, 2018 at 05:08:05PM +0000, Ian Jackson wrote:
> * Promise that we will set errno to ENOENT if the server is not
>   yet set up.
> * Arrange that all ENOENT returns other than from the read of ring-ref
>   are turned into EIO, logging when we do so.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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