From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH RFC tools 1/6] tools: Refactor "xentoollog" into its own library Date: Thu, 11 Jun 2015 12:35:12 +0100 Message-ID: <1434022512.30003.150.camel@citrix.com> References: <1433936188.30003.60.camel@citrix.com> <1433936205-21539-1-git-send-email-ian.campbell@citrix.com> <55796F0F.7040406@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55796F0F.7040406@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Jan Beulich Cc: wei.liu2@citrix.com, roger.pau@citrix.com, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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