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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/09/15 17:17, Ian Jackson wrote:
> 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.

It is fine (in principle) to be used internally.  Not in a public header
for what is supposed to be a clean API.

>
>> violates several principles of least supprise,
> This is just invective.

/me googles and discovered a new word.  I stand by my statement.

It requires the user to embed one structure inside another structure
(with half a undocumented magic name), under a specific undocumented
field name, and have three functions (with half undocumented magic
names) in scope with undocumented types.  It also mandates an original
copy of the half-magically-named struct in scope to be partially
initialised, just to have another copy created in the heap.

The error handling is also broken;  It will follow a wild function
pointer, if vtable is not the first element in the structure (again
undocumented).

This macro is not fit for any purpose it attempts to perform.

https://github.com/xenserver/xen-4.5.pg/blob/master/master/xenguest.patch#L609
is the correct way to use a setup like this, even if it does require
embedding other data alongside it.

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

Or better yet, ditch it completely.

All that needs to happen is for struct xentoollog_logger to have the
three function pointers filled in appropriately.  This in turn does not
mandate that the structure is allocated from the heap, or the prescribed
layout/naming scheme.

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

Pedantic is absolutely the correct answer.  It will cause gcc to reject
any non C compliant statements.

The APIs of each of these new libraries should be just as usable on
windows systems as Linux systems.

~Andrew