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?
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
