| Message ID | 1452802425-32603-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 14, 2016 at 08:13:45PM +0000, Andrew Cooper wrote: > XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the symbols > visible in its scope, and as such is only usable by its sole caller. > > Remove it. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com>
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? I think if you intend to remove something on this basis you need to be specific about what you believe the short comings are. > 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. > > Remove it. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Ian Campbell <Ian.Campbell@citrix.com> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > --- > tools/libs/toollog/include/xentoollog.h | 21 --------------------- > tools/libs/toollog/xtl_logger_stdio.c | 30 ++++++++++++++++++--------- > --- > 2 files changed, 18 insertions(+), 33 deletions(-) > > diff --git a/tools/libs/toollog/include/xentoollog.h > b/tools/libs/toollog/include/xentoollog.h > index 853e9c7..2b5bfcb 100644 > --- a/tools/libs/toollog/include/xentoollog.h > +++ b/tools/libs/toollog/include/xentoollog.h > @@ -112,25 +112,4 @@ void xtl_progress(struct xentoollog_logger *logger, > > const char *xtl_level_to_string(xentoollog_level); /* never fails */ > > - > -#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; > \ > -}); > - > - > #endif /* XENTOOLLOG_H */ > diff --git a/tools/libs/toollog/xtl_logger_stdio.c > b/tools/libs/toollog/xtl_logger_stdio.c > index 0cd9206..8bce1a7 100644 > --- a/tools/libs/toollog/xtl_logger_stdio.c > +++ b/tools/libs/toollog/xtl_logger_stdio.c > @@ -165,28 +165,34 @@ void > xtl_stdiostream_adjust_flags(xentoollog_logger_stdiostream *lg, > > xentoollog_logger_stdiostream *xtl_createlogger_stdiostream > (FILE *f, xentoollog_level min_level, unsigned flags) { > - xentoollog_logger_stdiostream newlogger; > > - newlogger.f = f; > - newlogger.min_level = min_level; > - newlogger.flags = flags; > + xentoollog_logger_stdiostream *nl = > + calloc(sizeof(xentoollog_logger_stdiostream), 1); > + > + if (!nl) > + return NULL; > + > + nl->vtable.vmessage = stdiostream_vmessage; > + nl->vtable.progress = stdiostream_progress; > + nl->vtable.destroy = stdiostream_destroy; > + > + nl->f = f; > + nl->min_level = min_level; > + nl->flags = flags; > > switch (flags & (XTL_STDIOSTREAM_PROGRESS_USE_CR | > XTL_STDIOSTREAM_PROGRESS_NO_CR)) { > - case XTL_STDIOSTREAM_PROGRESS_USE_CR: newlogger.progress_use_cr = 1; > break; > - case XTL_STDIOSTREAM_PROGRESS_NO_CR: newlogger.progress_use_cr = 0; > break; > + case XTL_STDIOSTREAM_PROGRESS_USE_CR: nl->progress_use_cr = 1; > break; > + case XTL_STDIOSTREAM_PROGRESS_NO_CR: nl->progress_use_cr = 0; > break; > case 0: > - newlogger.progress_use_cr = isatty(fileno(newlogger.f)) > 0; > + nl->progress_use_cr = isatty(fileno(nl->f)) > 0; > break; > default: > errno = EINVAL; > return 0; > } > > - if (newlogger.flags & XTL_STDIOSTREAM_SHOW_DATE) tzset(); > - > - newlogger.progress_erase_len = 0; > - newlogger.progress_last_percent = 0; > + if (nl->flags & XTL_STDIOSTREAM_SHOW_DATE) tzset(); > > - return XTL_NEW_LOGGER(stdiostream, newlogger); > + return nl; > }
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. > > 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. > >> 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. I stand firmly by my statement. ~Andrew
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.
Ian Campbell writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> 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.
Maybe I should submit a counter-patch providing documentation.
> > If vtable isn't the first element in the structure, it follows a wild
> > pointer on error.
This could be fixed.
> 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.
> BTW your patch removes the logging on failure to allocate, which should
> either be fixed or called out in the commit message.
I don't think this is a good idea.
Ian.
Ian Jackson writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> Ian Campbell writes ("Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()"):
> > > If vtable isn't the first element in the structure, it follows a wild
> > > pointer on error.
>
> This could be fixed.
Actually, no it couldn't because the vtable has to come first anyway.
Ian.
diff --git a/tools/libs/toollog/include/xentoollog.h b/tools/libs/toollog/include/xentoollog.h index 853e9c7..2b5bfcb 100644 --- a/tools/libs/toollog/include/xentoollog.h +++ b/tools/libs/toollog/include/xentoollog.h @@ -112,25 +112,4 @@ void xtl_progress(struct xentoollog_logger *logger, const char *xtl_level_to_string(xentoollog_level); /* never fails */ - -#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; \ -}); - - #endif /* XENTOOLLOG_H */ diff --git a/tools/libs/toollog/xtl_logger_stdio.c b/tools/libs/toollog/xtl_logger_stdio.c index 0cd9206..8bce1a7 100644 --- a/tools/libs/toollog/xtl_logger_stdio.c +++ b/tools/libs/toollog/xtl_logger_stdio.c @@ -165,28 +165,34 @@ void xtl_stdiostream_adjust_flags(xentoollog_logger_stdiostream *lg, xentoollog_logger_stdiostream *xtl_createlogger_stdiostream (FILE *f, xentoollog_level min_level, unsigned flags) { - xentoollog_logger_stdiostream newlogger; - newlogger.f = f; - newlogger.min_level = min_level; - newlogger.flags = flags; + xentoollog_logger_stdiostream *nl = + calloc(sizeof(xentoollog_logger_stdiostream), 1); + + if (!nl) + return NULL; + + nl->vtable.vmessage = stdiostream_vmessage; + nl->vtable.progress = stdiostream_progress; + nl->vtable.destroy = stdiostream_destroy; + + nl->f = f; + nl->min_level = min_level; + nl->flags = flags; switch (flags & (XTL_STDIOSTREAM_PROGRESS_USE_CR | XTL_STDIOSTREAM_PROGRESS_NO_CR)) { - case XTL_STDIOSTREAM_PROGRESS_USE_CR: newlogger.progress_use_cr = 1; break; - case XTL_STDIOSTREAM_PROGRESS_NO_CR: newlogger.progress_use_cr = 0; break; + case XTL_STDIOSTREAM_PROGRESS_USE_CR: nl->progress_use_cr = 1; break; + case XTL_STDIOSTREAM_PROGRESS_NO_CR: nl->progress_use_cr = 0; break; case 0: - newlogger.progress_use_cr = isatty(fileno(newlogger.f)) > 0; + nl->progress_use_cr = isatty(fileno(nl->f)) > 0; break; default: errno = EINVAL; return 0; } - if (newlogger.flags & XTL_STDIOSTREAM_SHOW_DATE) tzset(); - - newlogger.progress_erase_len = 0; - newlogger.progress_last_percent = 0; + if (nl->flags & XTL_STDIOSTREAM_SHOW_DATE) tzset(); - return XTL_NEW_LOGGER(stdiostream, newlogger); + return nl; }
XTL_NEW_LOGGER() makes a number of unreasonable assumptions about the symbols visible in its scope, and as such is only usable by its sole caller. Remove it. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Campbell <Ian.Campbell@citrix.com> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- tools/libs/toollog/include/xentoollog.h | 21 --------------------- tools/libs/toollog/xtl_logger_stdio.c | 30 ++++++++++++++++++------------ 2 files changed, 18 insertions(+), 33 deletions(-)