[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.