Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

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

On Tue, 2016-01-19 at 16:40 +0000, Andrew Cooper wrote:
> On 19/01/16 16:24, Ian Campbell wrote:
> > On Thu, 2016-01-14 at 20:13 +0000, Andrew Cooper wrote:
> > > XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the
> > > symbols
> > > visible in its scope,
> > It assumes that the function names to fill in the vtable and the type
> > name
> > are related, that hardly seems totally "unreasonable". What else does
> > it
> > assume that makes it unreasonable?
> 
> We have already had this argument once, the result being "patch
> welcome".  Here one is.
> 
> Not only does it assume certain names, it is tokenised with a magic 3rd
> identifier.
> 
> It also assumes the presence of a structure member named vtable.

The underlying issue with all of these is the _undocumented_ nature of the
assumptions, which is certainly a bug, however those assumptions are not in
themselves "unreasonable" as was claimed.

> > I think if you intend to remove something on this basis you need to be
> > specific about what you believe the short comings are.
> 
> GNUism in header file claiming C99 strictness
> 
> If vtable isn't the first element in the structure, it follows a wild
> pointer on error.

Thank you. Both of these and the lack of documentation should have been
spelled out in the original commit message as reasons for the removal.

> > >  and as such is only usable by its sole caller.
> > "not usable by every imaginable caller" is not the same as "usable by
> > one
> > single possible caller", I think you are overstating the case here.
> 
> It is genuinely harder to reverse engineer how to use that macro than to
> opencode a sane alternative.

A consequence of the lack of documentation rather than any inherent
unreasonableness of the provision of such a helper.

BTW your patch removes the logging on failure to allocate, which should
either be fixed or called out in the commit message.

> I stand firmly by my statement.

You might reasonably stand by your actual reasons for removing this macro,
but the original commit message didn't contain them.

Ian.

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