xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Ian Campbell <ian.campbell@citrix.com>, xen-devel@lists.xen.org
Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com,
	ian.jackson@eu.citrix.com, roger.pau@citrix.com
Subject: Re: [PATCH RFC tools 1/6] tools: Refactor "xentoollog" into its own library
Date: Thu, 11 Jun 2015 12:20:47 +0100	[thread overview]
Message-ID: <55796F0F.7040406@citrix.com> (raw)
In-Reply-To: <1433936205-21539-1-git-send-email-ian.campbell@citrix.com>

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

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

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.

~Andrew

  reply	other threads:[~2015-06-11 11:20 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-10 11:36 [PATCH RFC 0/6+2+2] Begin to disentangle libxenctrl and provide some stable libraries Ian Campbell
2015-06-10 11:36 ` [PATCH RFC tools 1/6] tools: Refactor "xentoollog" into its own library Ian Campbell
2015-06-11 11:20   ` Andrew Cooper [this message]
2015-06-11 11:35     ` Ian Campbell
2015-06-11 12:06       ` Jan Beulich
2015-06-11 12:21         ` Ian Campbell
2015-09-21 16:17     ` Ian Jackson
2015-09-21 17:03       ` Andrew Cooper
2015-09-21 17:13         ` Ian Jackson
2015-09-21 17:30           ` Andrew Cooper
2015-09-22  8:39             ` Ian Campbell
2015-06-10 11:36 ` [PATCH RFC tools 2/6] tools: Link in-tree libvchan users against libxenvchan.so Ian Campbell
2015-06-10 11:36 ` [PATCH RFC tools 3/6] tools: Do not add top-level tools dir to include path Ian Campbell
2015-06-10 11:36 ` [PATCH RFC tools 4/6] tools/libxc: Remove osdep indirection for xc_evtchn Ian Campbell
2015-06-10 11:36 ` [PATCH RFC tools 5/6] tools: Refactor /dev/xen/evtchn wrappers into libxenevtchn Ian Campbell
2015-06-10 16:29   ` David Vrabel
2015-06-11  8:58     ` Ian Campbell
2015-06-10 17:16   ` Andrew Cooper
2015-06-11  9:03     ` Ian Campbell
2015-06-10 11:36 ` [PATCH RFC tools 6/6] Cleanup SHLIBDEPS Ian Campbell
2015-06-10 11:37 ` [PATCH RFC qemu-trad 1/2] qemu-xen-traditional: Use xentoollog as a separate library Ian Campbell
2015-06-10 15:57   ` Ian Jackson
2015-06-11  8:59     ` Ian Campbell
2015-06-10 11:37 ` [PATCH RFC qemu-trad 2/2] qemu-xen-traditional: Use libxenevtchn Ian Campbell
2015-06-10 11:37 ` [PATCH RFC mini-os 1/2] mini-os: Include libxentoollog with libxc Ian Campbell
2015-06-10 11:37 ` [PATCH RFC mini-os 2/2] mini-os: Include libxenevtchn " Ian Campbell
2015-06-10 16:01 ` [PATCH RFC 0/6+2+2] Begin to disentangle libxenctrl and provide some stable libraries Ian Jackson
2015-06-10 16:15   ` Wei Liu
2015-06-11 10:01     ` Antti Kantee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55796F0F.7040406@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=ian.campbell@citrix.com \
    --cc=ian.jackson@eu.citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=stefano.stabellini@eu.citrix.com \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).