[PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Andrew Cooper]

XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the symbols
visible in its scope, and as such is only usable by its sole caller.

Remove it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libs/toollog/include/xentoollog.h | 21 ---------------------
 tools/libs/toollog/xtl_logger_stdio.c   | 30 ++++++++++++++++++------------
 2 files changed, 18 insertions(+), 33 deletions(-)

diff --git a/tools/libs/toollog/include/xentoollog.h b/tools/libs/toollog/include/xentoollog.h
index 853e9c7..2b5bfcb 100644
--- a/tools/libs/toollog/include/xentoollog.h
+++ b/tools/libs/toollog/include/xentoollog.h
@@ -112,25 +112,4 @@ void xtl_progress(struct xentoollog_logger *logger,
 
 const char *xtl_level_to_string(xentoollog_level); /* never fails */
 
-
-#define XTL_NEW_LOGGER(LOGGER,buffer) ({                                \
-    xentoollog_logger_##LOGGER *new_consumer;                           \
-                                                                        \
-    (buffer).vtable.vmessage = LOGGER##_vmessage;                       \
-    (buffer).vtable.progress = LOGGER##_progress;                       \
-    (buffer).vtable.destroy  = LOGGER##_destroy;                        \
-                                                                        \
-    new_consumer = malloc(sizeof(*new_consumer));                       \
-    if (!new_consumer) {                                                \
-        xtl_log((xentoollog_logger*)&buffer,                            \
-                XTL_CRITICAL, errno, "xtl",                             \
-                "failed to allocate memory for new message logger");    \
-    } else {                                                            \
-        *new_consumer = buffer;                                         \
-    }                                                                   \
-                                                                        \
-    new_consumer;                                                       \
-});
-
-
 #endif /* XENTOOLLOG_H */
diff --git a/tools/libs/toollog/xtl_logger_stdio.c b/tools/libs/toollog/xtl_logger_stdio.c
index 0cd9206..8bce1a7 100644
--- a/tools/libs/toollog/xtl_logger_stdio.c
+++ b/tools/libs/toollog/xtl_logger_stdio.c
@@ -165,28 +165,34 @@ void xtl_stdiostream_adjust_flags(xentoollog_logger_stdiostream *lg,
 
 xentoollog_logger_stdiostream *xtl_createlogger_stdiostream
         (FILE *f, xentoollog_level min_level, unsigned flags) {
-    xentoollog_logger_stdiostream newlogger;
 
-    newlogger.f = f;
-    newlogger.min_level = min_level;
-    newlogger.flags = flags;
+    xentoollog_logger_stdiostream *nl =
+        calloc(sizeof(xentoollog_logger_stdiostream), 1);
+
+    if (!nl)
+        return NULL;
+
+    nl->vtable.vmessage = stdiostream_vmessage;
+    nl->vtable.progress = stdiostream_progress;
+    nl->vtable.destroy  = stdiostream_destroy;
+
+    nl->f = f;
+    nl->min_level = min_level;
+    nl->flags = flags;
 
     switch (flags & (XTL_STDIOSTREAM_PROGRESS_USE_CR |
                      XTL_STDIOSTREAM_PROGRESS_NO_CR)) {
-    case XTL_STDIOSTREAM_PROGRESS_USE_CR: newlogger.progress_use_cr = 1; break;
-    case XTL_STDIOSTREAM_PROGRESS_NO_CR:  newlogger.progress_use_cr = 0; break;
+    case XTL_STDIOSTREAM_PROGRESS_USE_CR: nl->progress_use_cr = 1; break;
+    case XTL_STDIOSTREAM_PROGRESS_NO_CR:  nl->progress_use_cr = 0; break;
     case 0:
-        newlogger.progress_use_cr = isatty(fileno(newlogger.f)) > 0;
+        nl->progress_use_cr = isatty(fileno(nl->f)) > 0;
         break;
     default:
         errno = EINVAL;
         return 0;
     }
 
-    if (newlogger.flags & XTL_STDIOSTREAM_SHOW_DATE) tzset();
-
-    newlogger.progress_erase_len = 0;
-    newlogger.progress_last_percent = 0;
+    if (nl->flags & XTL_STDIOSTREAM_SHOW_DATE) tzset();
 
-    return XTL_NEW_LOGGER(stdiostream, newlogger);
+    return nl;
 }
-- 
2.1.4

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Wei Liu]

On Thu, Jan 14, 2016 at 08:13:45PM +0000, Andrew Cooper wrote:
> XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the symbols
> visible in its scope, and as such is only usable by its sole caller.
> 
> Remove it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Ian Campbell]

On Thu, 2016-01-14 at 20:13 +0000, Andrew Cooper wrote:
> XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the symbols
> visible in its scope,

It assumes that the function names to fill in the vtable and the type name
are related, that hardly seems totally "unreasonable". What else does it
assume that makes it unreasonable?

I think if you intend to remove something on this basis you need to be
specific about what you believe the short comings are.

>  and as such is only usable by its sole caller.

"not usable by every imaginable caller" is not the same as "usable by one
single possible caller", I think you are overstating the case here.

> 
> Remove it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libs/toollog/include/xentoollog.h | 21 ---------------------
>  tools/libs/toollog/xtl_logger_stdio.c   | 30 ++++++++++++++++++---------
> ---
>  2 files changed, 18 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/libs/toollog/include/xentoollog.h
> b/tools/libs/toollog/include/xentoollog.h
> index 853e9c7..2b5bfcb 100644
> --- a/tools/libs/toollog/include/xentoollog.h
> +++ b/tools/libs/toollog/include/xentoollog.h
> @@ -112,25 +112,4 @@ void xtl_progress(struct xentoollog_logger *logger,
>  
>  const char *xtl_level_to_string(xentoollog_level); /* never fails */
>  
> -
> -#define XTL_NEW_LOGGER(LOGGER,buffer)
> ({                                \
> -    xentoollog_logger_##LOGGER
> *new_consumer;                           \
> -                                                                        
> \
> -    (buffer).vtable.vmessage =
> LOGGER##_vmessage;                       \
> -    (buffer).vtable.progress =
> LOGGER##_progress;                       \
> -    (buffer).vtable.destroy  =
> LOGGER##_destroy;                        \
> -                                                                        
> \
> -    new_consumer =
> malloc(sizeof(*new_consumer));                       \
> -    if (!new_consumer)
> {                                                \
> -        xtl_log((xentoollog_logger*)&buffer,                            
> \
> -                XTL_CRITICAL, errno,
> "xtl",                             \
> -                "failed to allocate memory for new message
> logger");    \
> -    } else
> {                                                            \
> -        *new_consumer =
> buffer;                                         \
> -    }                                                                   
> \
> -                                                                        
> \
> -    new_consumer;                                                       
> \
> -});
> -
> -
>  #endif /* XENTOOLLOG_H */
> diff --git a/tools/libs/toollog/xtl_logger_stdio.c
> b/tools/libs/toollog/xtl_logger_stdio.c
> index 0cd9206..8bce1a7 100644
> --- a/tools/libs/toollog/xtl_logger_stdio.c
> +++ b/tools/libs/toollog/xtl_logger_stdio.c
> @@ -165,28 +165,34 @@ void
> xtl_stdiostream_adjust_flags(xentoollog_logger_stdiostream *lg,
>  
>  xentoollog_logger_stdiostream *xtl_createlogger_stdiostream
>          (FILE *f, xentoollog_level min_level, unsigned flags) {
> -    xentoollog_logger_stdiostream newlogger;
>  
> -    newlogger.f = f;
> -    newlogger.min_level = min_level;
> -    newlogger.flags = flags;
> +    xentoollog_logger_stdiostream *nl =
> +        calloc(sizeof(xentoollog_logger_stdiostream), 1);
> +
> +    if (!nl)
> +        return NULL;
> +
> +    nl->vtable.vmessage = stdiostream_vmessage;
> +    nl->vtable.progress = stdiostream_progress;
> +    nl->vtable.destroy  = stdiostream_destroy;
> +
> +    nl->f = f;
> +    nl->min_level = min_level;
> +    nl->flags = flags;
>  
>      switch (flags & (XTL_STDIOSTREAM_PROGRESS_USE_CR |
>                       XTL_STDIOSTREAM_PROGRESS_NO_CR)) {
> -    case XTL_STDIOSTREAM_PROGRESS_USE_CR: newlogger.progress_use_cr = 1;
> break;
> -    case XTL_STDIOSTREAM_PROGRESS_NO_CR:  newlogger.progress_use_cr = 0;
> break;
> +    case XTL_STDIOSTREAM_PROGRESS_USE_CR: nl->progress_use_cr = 1;
> break;
> +    case XTL_STDIOSTREAM_PROGRESS_NO_CR:  nl->progress_use_cr = 0;
> break;
>      case 0:
> -        newlogger.progress_use_cr = isatty(fileno(newlogger.f)) > 0;
> +        nl->progress_use_cr = isatty(fileno(nl->f)) > 0;
>          break;
>      default:
>          errno = EINVAL;
>          return 0;
>      }
>  
> -    if (newlogger.flags & XTL_STDIOSTREAM_SHOW_DATE) tzset();
> -
> -    newlogger.progress_erase_len = 0;
> -    newlogger.progress_last_percent = 0;
> +    if (nl->flags & XTL_STDIOSTREAM_SHOW_DATE) tzset();
>  
> -    return XTL_NEW_LOGGER(stdiostream, newlogger);
> +    return nl;
>  }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Andrew Cooper]

On 19/01/16 16:24, Ian Campbell wrote:
> On Thu, 2016-01-14 at 20:13 +0000, Andrew Cooper wrote:
>> XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the symbols
>> visible in its scope,
> It assumes that the function names to fill in the vtable and the type name
> are related, that hardly seems totally "unreasonable". What else does it
> assume that makes it unreasonable?

We have already had this argument once, the result being "patch
welcome".  Here one is.

Not only does it assume certain names, it is tokenised with a magic 3rd
identifier.

It also assumes the presence of a structure member named vtable.

>
> I think if you intend to remove something on this basis you need to be
> specific about what you believe the short comings are.

GNUism in header file claiming C99 strictness

If vtable isn't the first element in the structure, it follows a wild
pointer on error.

>
>>  and as such is only usable by its sole caller.
> "not usable by every imaginable caller" is not the same as "usable by one
> single possible caller", I think you are overstating the case here.

It is genuinely harder to reverse engineer how to use that macro than to
opencode a sane alternative.

I stand firmly by my statement.

~Andrew

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Ian Campbell]

On Tue, 2016-01-19 at 16:40 +0000, Andrew Cooper wrote:
> On 19/01/16 16:24, Ian Campbell wrote:
> > On Thu, 2016-01-14 at 20:13 +0000, Andrew Cooper wrote:
> > > XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the
> > > symbols
> > > visible in its scope,
> > It assumes that the function names to fill in the vtable and the type
> > name
> > are related, that hardly seems totally "unreasonable". What else does
> > it
> > assume that makes it unreasonable?
> 
> We have already had this argument once, the result being "patch
> welcome".  Here one is.
> 
> Not only does it assume certain names, it is tokenised with a magic 3rd
> identifier.
> 
> It also assumes the presence of a structure member named vtable.

The underlying issue with all of these is the _undocumented_ nature of the
assumptions, which is certainly a bug, however those assumptions are not in
themselves "unreasonable" as was claimed.

> > I think if you intend to remove something on this basis you need to be
> > specific about what you believe the short comings are.
> 
> GNUism in header file claiming C99 strictness
> 
> If vtable isn't the first element in the structure, it follows a wild
> pointer on error.

Thank you. Both of these and the lack of documentation should have been
spelled out in the original commit message as reasons for the removal.

> > >  and as such is only usable by its sole caller.
> > "not usable by every imaginable caller" is not the same as "usable by
> > one
> > single possible caller", I think you are overstating the case here.
> 
> It is genuinely harder to reverse engineer how to use that macro than to
> opencode a sane alternative.

A consequence of the lack of documentation rather than any inherent
unreasonableness of the provision of such a helper.

BTW your patch removes the logging on failure to allocate, which should
either be fixed or called out in the commit message.

> I stand firmly by my statement.

You might reasonably stand by your actual reasons for removing this macro,
but the original commit message didn't contain them.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> The underlying issue with all of these is the _undocumented_ nature of the
> assumptions, which is certainly a bug, however those assumptions are not in
> themselves "unreasonable" as was claimed.

Maybe I should submit a counter-patch providing documentation.

> > If vtable isn't the first element in the structure, it follows a wild
> > pointer on error.

This could be fixed.

> Thank you. Both of these and the lack of documentation should have been
> spelled out in the original commit message as reasons for the removal.

> BTW your patch removes the logging on failure to allocate, which should
> either be fixed or called out in the commit message.

I don't think this is a good idea.

Ian.

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> Ian Campbell writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> > > If vtable isn't the first element in the structure, it follows a wild
> > > pointer on error.
> 
> This could be fixed.

Actually, no it couldn't because the vtable has to come first anyway.

Ian.

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> Ian Campbell writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> > The underlying issue with all of these is the _undocumented_ nature of the
> > assumptions, which is certainly a bug, however those assumptions are not in
> > themselves "unreasonable" as was claimed.
> 
> Maybe I should submit a counter-patch providing documentation.

I think this macro is useful because if you wanted to write (say)
xtl_logger_syslog, you would want to use it to help you with some
boilerplate.

Ian.

>From f749eea51c35c787b8ca7514a21ac145e2946ff8 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Tue, 19 Jan 2016 17:29:30 +0000
Subject: [PATCH] xentoollog: Document XTL_NEW_LOGGER convenience macro

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libs/toollog/include/xentoollog.h |   53 +++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/tools/libs/toollog/include/xentoollog.h b/tools/libs/toollog/include/xentoollog.h
index 853e9c7..95f7482 100644
--- a/tools/libs/toollog/include/xentoollog.h
+++ b/tools/libs/toollog/include/xentoollog.h
@@ -113,6 +113,59 @@ void xtl_progress(struct xentoollog_logger *logger,
 const char *xtl_level_to_string(xentoollog_level); /* never fails */
 
 
+/*
+ * To use this macro:
+ *
+ *   Define your own logger struct, containing the vtable.
+ *
+ *       typedef struct {
+ *            xentoollog_logger vtable; // must come first
+ *            [ state your logger needs ]
+ *       } xentoolog_logger_mine;
+ *
+ *   Write the logging functions:
+ *
+ *       static void mine_vmessage([ see above ]);
+ *       static void mine_progress([ see above ]);
+ *       static void mine_destroy(struct xentoollog_logger *logger);
+ *
+ *   Write a constructor:
+ *
+ *       mine_xentoollog_logger *tl_createlogger_mine([whatever]) {
+ *           mine_xentoolllog_logger newlogger;
+ *
+ *           [ fill in fields of newlogger ]
+ *
+ *           return XTL_NEW_LOGGER(mine, newlogger);
+ *       }
+ *
+ *   If newlogger contains resources that might need to be released,
+ *   the constructor must check the return value from XTL_NEW_LOGGER:
+ *   if it is NULL, the constructor must release the resources.
+ *
+ *
+ * Formally:
+ *
+ *   xentoollog_logger_MINE*
+ *   XTL_NEW_LOGGER(MINE, xentoollog_logger_MINE contents);
+ *
+ *     Fills in contents.vtable.  Allocates a new struct.  Copies
+ *     contents into it.  Finally, returns a pointer to the copy.
+ *
+ *     If allocation fails, uses contents to report this failure, and
+ *     returns NULL.
+ *
+ *   Expects that xentoollog_logger_MINE is a struct whose
+ *   first member is
+ *            xentoollog_logger vtable;
+ *
+ *   Expects that
+ *      MINE_vmessage
+ *      MINE_progress
+ *      MINE_destroy
+ *   are in scope, with types compatible with the vtable members.
+ *
+ */
 #define XTL_NEW_LOGGER(LOGGER,buffer) ({                                \
     xentoollog_logger_##LOGGER *new_consumer;                           \
                                                                         \
-- 
1.7.10.4

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Andrew Cooper]

On 19/01/16 17:36, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
>> Ian Campbell writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
>>> The underlying issue with all of these is the _undocumented_ nature of the
>>> assumptions, which is certainly a bug, however those assumptions are not in
>>> themselves "unreasonable" as was claimed.
>> Maybe I should submit a counter-patch providing documentation.
> I think this macro is useful because if you wanted to write (say)
> xtl_logger_syslog, you would want to use it to help you with some
> boilerplate.

WTF? Even documented, the behaviour of this macro is insane, which is
why I am trying to kill it.  After this, I will also be fixing the gross
pointer abuse which exists in the xentoollog internals, before the ABI
becomes fixed in 4.7.

There should be no place for code like this, and certainly not in the
clean API/ABI we are trying to create out of the mess which is libxc.

Irrespective of whether you disagree with my opinions here, xentoollog.h
is specified to be C99 -strict, meaning no GNUisms.

~Andrew

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> On 19/01/16 17:36, Ian Jackson wrote:
> > I think this macro is useful because if you wanted to write (say)
> > xtl_logger_syslog, you would want to use it to help you with some
> > boilerplate.
> 
> WTF? Even documented, the behaviour of this macro is insane, which is
> why I am trying to kill it.  After this, I will also be fixing the gross
> pointer abuse which exists in the xentoollog internals, before the ABI
> becomes fixed in 4.7.

I think the behaviour of this macro is perfectly sane.

I think your reference to `gross pointer abuse' is to the casting from
the specific to the generic struct.  This is a completely standard
technique for oopy stuff in C.  Here is a whole library (quite a nice
neat library, in fact) that uses it:
   http://www.lysator.liu.se/liboop/
   http://www.lysator.liu.se/liboop/ref.html

> Irrespective of whether you disagree with my opinions here, xentoollog.h
> is specified to be C99 -strict, meaning no GNUisms.

Specified where ?

Anyway, there is no requirement to use this macro.  If someone wants
to write a strict C99 xtl logger then they can do it by hand.  (I
predict that no-one will want to do that.)  So there is clearly no
actual reason why this macro ought to be pure C99.

Ian.

Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

[Ian Campbell]

On Tue, 2016-01-19 at 17:58 +0000, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH] tools/toollog: Drop
> XTL_NEW_LOGGER()"):
> > On 19/01/16 17:36, Ian Jackson wrote:
> > > I think this macro is useful because if you wanted to write (say)
> > > xtl_logger_syslog, you would want to use it to help you with some
> > > boilerplate.
> > 
> > WTF? Even documented, the behaviour of this macro is insane, which is
> > why I am trying to kill it.  After this, I will also be fixing the
> > gross
> > pointer abuse which exists in the xentoollog internals, before the ABI
> > becomes fixed in 4.7.
> 
> I think the behaviour of this macro is perfectly sane.

It's clearly not "insane", that's for sure, and statements of such opinions
along those lines in the commit message (as oppose to concrete issues) are
why I found the original patch unsuitable to apply.

> I think your reference to `gross pointer abuse' is to the casting from
> the specific to the generic struct.  This is a completely standard
> technique for oopy stuff in C.  Here is a whole library (quite a nice
> neat library, in fact) that uses it:
>    http://www.lysator.liu.se/liboop/
>    http://www.lysator.liu.se/liboop/ref.html
> 
> > Irrespective of whether you disagree with my opinions here,
> > xentoollog.h
> > is specified to be C99 -strict, meaning no GNUisms.
> 
> Specified where ?

Sort of in "tools: Arrange to check public headers for ANSI compatiblity"
(yet to be applied) where I add checks that the headers are clean per gcc's
-ansi flag, which is equivalent to -std=c90.

This was done to support users of the library on platforms with lagging
compilers, specifically MS VCC lacks some stuff more modern C stuff (like
variable length arrays IIRC, the details were in one of the threads on that
patch)

This check doesn't cover #defines though.

> Anyway, there is no requirement to use this macro.  If someone wants
> to write a strict C99 xtl logger then they can do it by hand.  (I
> predict that no-one will want to do that.)  So there is clearly no
> actual reason why this macro ought to be pure C99.

Now that the macro is documented I think the GNUism is the only remaining
concern which has been put forward which I think needs addressing any
further and I agree with the logic that it is a #define so if you want to
use an non-gnu compiler you don't get the handy macro.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel