diff mbox

tools/toollog: Drop XTL_NEW_LOGGER()

Message ID 22174.29744.755487.990490@mariner.uk.xensource.com
State New, archived
Headers show

Commit Message

Ian Jackson Jan. 19, 2016, 5:36 p.m. UTC
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(+)

Comments

Andrew Cooper Jan. 19, 2016, 5:45 p.m. UTC | #1
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
Ian Jackson Jan. 19, 2016, 5:58 p.m. UTC | #2
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.
Ian Campbell Jan. 20, 2016, 5:21 p.m. UTC | #3
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.
diff mbox

Patch

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;                           \
                                                                         \