On Thu, Nov 22, 2018 at 12:45:20PM -0600, Rob Herring wrote:
> On Thu, Nov 22, 2018 at 5:16 AM David Gibson
> <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Nov 21, 2018 at 03:53:58PM -0600, Rob Herring wrote:
> > > Printing to stderr as we build up the check message results in
> > > interleaving of messages when multiple instances of dtc are running.
> > > Change the message output to use an intermediate buffer for constructing
> > > the message and then output the message to stderr with a single fputs.
> > >
> > > While perhaps there is no guarantee that fputs will be atomic, this gets
> > > rid of any interleaved output that previously occurred on Linux.
> > >
> > > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> >
> > I can see that this would be beneficial, but..
> >
> > > ---
> > > checks.c | 29 +++++++++++++++++++++--------
> > > 1 file changed, 21 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/checks.c b/checks.c
> > > index 4d8dffd36b6e..e60940d2af86 100644
> > > --- a/checks.c
> > > +++ b/checks.c
> > > @@ -72,29 +72,42 @@ struct check {
> > > #define CHECK(nm_, fn_, d_, ...) \
> > > CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
> > >
> > > +#define MAX_MSG_LEN 1024
> >
> > I really hate arbitrary buffer limits.
> >
> > We are already have an xasprintf() function, so lets use that.
>
> Okay, that's what I wrote first...
>
> It gets kind of ugly because you end up with a string for each segment
> and then assembled at the end. This is really problematic for the
> "also defined at ..." strings as we don't know how many we have. So
> count how many we have, allocate an array of char* ptrs, and then loop
> again?
Ok, sounds like you want an xasprintf_append() helper,
new = xasprintf("%s" fmt, old, ...)
free(old)
return new
Then you can build it up piece by piece as you do here. Yeah, it's a
bunch of extra copys and reallocs, but it's an error path in a utility
that's rarely performance critical to begin with.
>
> Rob
>
>
> >
> > > +
> > > static inline void PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti,
> > > struct node *node,
> > > struct property *prop,
> > > const char *fmt, ...)
> > > {
> > > va_list ap;
> > > + char buffer[MAX_MSG_LEN];
> > > + char *str = buffer;
> > > + char *end = str + MAX_MSG_LEN;
> > >
> > > if (!(c->warn && (quiet < 1)) && !(c->error && (quiet < 2)))
> > > return;
> > >
> > > - fprintf(stderr, "%s: %s (%s): ",
> > > - strcmp(dti->outname, "-") ? dti->outname : "<stdout>",
> > > - (c->error) ? "ERROR" : "Warning", c->name);
> > > + str += snprintf(str, end - str, "%s: %s (%s): ",
> > > + strcmp(dti->outname, "-") ? dti->outname : "<stdout>",
> > > + (c->error) ? "ERROR" : "Warning", c->name);
> > > + assert(str < end);
> > > +
> > > if (node) {
> > > - fprintf(stderr, "%s", node->fullpath);
> > > if (prop)
> > > - fprintf(stderr, ":%s", prop->name);
> > > - fputs(": ", stderr);
> > > + str += snprintf(str, end - str, "%s:%s: ", node->fullpath, prop->name);
> > > + else
> > > + str += snprintf(str, end - str, "%s: ", node->fullpath);
> > > + assert(str < end);
> > > }
> > > +
> > > va_start(ap, fmt);
> > > - vfprintf(stderr, fmt, ap);
> > > + str += vsnprintf(str, end - str, fmt, ap);
> > > + assert(str < end);
> > > va_end(ap);
> > > - fprintf(stderr, "\n");
> > > +
> > > + strcpy(str, "\n");
> > > +
> > > + fputs(buffer, stderr);
> > > }
> > >
> > > #define FAIL(c, dti, node, ...) \
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
