Re: [Xen-devel] [PATCH RFC tools 1/6] tools: Refactor "xentoollog" into its own library

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH RFC tools 1/6] tools: Refactor 
"xentoollog" into its own library"):
> On 10/06/15 12:36, Ian Campbell wrote:
> > +
> > +#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;                                                       \
> > +});

Ian Campbell just pointed me at this.

> This macro should be ditched.
> 
> It is a gnu-ism which shouldn't be present in the public library header,

Do you mean that statement expressions (originally a GNU extension)
should be avoided in tools code ?  A quick git-grep discovered that
xenctrl already contains numerous statement expressions.

> violates several principles of least supprise,

This is just invective.

> and can literally only be
> used by its sole user in xtl_logger_stdio.c because of its internal
> expectations of xentoollog_logger_stdiostream.  (Its sole user could do
> the above in a cleaner manner anyway.)

There is only one place in tree that calls this because there are only
two in-tree loggers and the other one
(tools/ocaml/libs/xentoollog/xentoollog_stubs.c) chose not to use it.
It seems to me that stub_xtl_create_logger could use this macro.

> As part of the tidyup, we should choose a particular C standard (89,
> probably) and ensure that the API/ABI complies with `gcc -std=c$VER
> -pedantic`.  This will help to provide a consistent API on other
> platforms (I seem to recall an effort to port libvchan to windows.)

-pedantic is certainly a bad idea.

> As another thought, it would also be a good time to sort out a
> consistent coding style, although that doesn't necessarily need to be
> folded into the split-out patch.  The current source is very mixed when
> it comes to coding style.

This sounds like it's going to lead to a lot of bikeshedding.  If
there are components with internally inconsistent coding styles that
should be fixed.  But I don't think it is a good idea to go around
reformatting areas that are coherent to make them like other areas
which are differently coherent.

Ian.

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