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

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-06-11 at 12:20 +0100, Andrew Cooper wrote:
> 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;                                                       \
> > +});
> 
> This macro should be ditched.
> 
> It is a gnu-ism which shouldn't be present in the public library header,
> violates several principles of least supprise, 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.)

Ian? Do you agree?

> 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.)

Shall we just follow what we do for xen/include/public i.e.
        $(CC) -x c -ansi -Wall -Werror
? It seems sensible that the two should follow similar rules.

Or if not shall we change the requirements for xen/include/public to
match? (Jan CCd for comments)

> 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.

True, but this is already quite a big job, with lots of moving parts
already. If I find time I could do a pass, but I don't think it is super
ciritical