Re: [Xen-devel] [PATCH 01/35] libxl: add LIBXL_LOGD_* and LOG*D function families.

[Cedric Bosdonnat <cbosdonnat@xxxxxxxx>]

On Thu, 2016-11-17 at 14:41 +0000, Wei Liu wrote:
> On Tue, Nov 15, 2016 at 11:18:39AM +0100, Cédric Bosdonnat wrote:
> > From: Cédric Bosdonnat <cedric.bosdonnat@xxxxxxx>
> > 
> > These functions should be used to log messages when the domain
> > id is known. libxl__log will now prepend the log message by
> > "Domain %PRIu32:" if the domain id is a valid one.
> > 
> > This aims at helping consumers filter logs on domain IDs.
> > 
> > Signed-off-by: Cédric Bosdonnat <cbosdonnat@xxxxxxxx>
> > ---
> >  tools/libxl/libxl_event.c    |  6 +++---
> >  tools/libxl/libxl_internal.c | 16 ++++++++++------
> >  tools/libxl/libxl_internal.h | 38 +++++++++++++++++++++++++++++++-------
> >  3 files changed, 44 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> > index 02b39e6..fc9bdc9 100644
> > --- a/tools/libxl/libxl_event.c
> > +++ b/tools/libxl/libxl_event.c
> > @@ -1362,7 +1362,7 @@ void libxl__event_disaster(libxl__egc *egc, const 
> > char *msg, int errnoval,
> >  {
> >      EGC_GC;
> >  
> > -    libxl__log(CTX, XTL_CRITICAL, errnoval, file, line, func,
> > +    libxl__log(CTX, XTL_CRITICAL, errnoval, file, line, func, 
> > INVALID_DOMID,
> >                 "DISASTER in event loop: %s%s%s%s",
> >                 msg,
> >                 type ? " (relates to event type " : "",
> > @@ -1943,7 +1943,7 @@ libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t 
> > domid,
> >          ao->poller = libxl__poller_get(&ao->gc);
> >          if (!ao->poller) goto out;
> >      }
> > -    libxl__log(ctx,XTL_DEBUG,-1,file,line,func,
> > +    libxl__log(ctx,XTL_DEBUG,-1,file,line,func,domid,
> >                 "ao %p: create: how=%p callback=%p poller=%p",
> >                 ao, how, ao->how.callback, ao->poller);
> >  
> > @@ -1968,7 +1968,7 @@ int libxl__ao_inprogress(libxl__ao *ao,
> >      assert(ao->in_initiator);
> >      ao->constructing = 0;
> >  
> > -    libxl__log(CTX,XTL_DEBUG,-1,file,line,func,
> > +    libxl__log(CTX,XTL_DEBUG,-1,file,line,func,INVALID_DOMID,
> 
> You should be able to use ao->domid here, right?

Oh indeed. I discovered this one after writing this patch and forgot
about it here. Could it be possible that ao->domid is not the publicly
known domain id?

> >                 "ao %p: inprogress: poller=%p, flags=%s%s%s%s",
> >                 ao, ao->poller,
> >                 ao->constructing ? "o" : "",
> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index 448dd61..78ed2f6 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -22,7 +22,7 @@ void libxl__alloc_failed(libxl_ctx *ctx, const char *func,
> >  #define M "libxl: FATAL ERROR: memory allocation failure"
> >  #define L (size ? M " (%s, %lu x %lu)\n" : M " (%s)\n"), \
> >            func, (unsigned long)nmemb, (unsigned long)size
> > -    libxl__log(ctx, XTL_CRITICAL, ENOMEM, 0,0, func, L);
> > +    libxl__log(ctx, XTL_CRITICAL, ENOMEM, 0,0, func, INVALID_DOMID, L);
> >      fprintf(stderr, L);
> >      fflush(stderr);
> >      _exit(-1);
> > @@ -202,7 +202,7 @@ char *libxl__dirname(libxl__gc *gc, const char *s)
> >  
> >  void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
> >               const char *file, int line, const char *func,
> > -             const char *fmt, va_list ap)
> > +             uint32_t domid, const char *fmt, va_list ap)
> >  {
> >      /* WARNING this function may not call any libxl-provided
> >       * memory allocation function, as those may
> > @@ -211,6 +211,7 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level 
> > msglevel, int errnoval,
> >      char *base = NULL;
> >      int rc, esave;
> >      char fileline[256];
> > +    char domain[256];
> >  
> >      esave = errno;
> >  
> > @@ -221,22 +222,25 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level 
> > msglevel, int errnoval,
> >      if (file) snprintf(fileline, sizeof(fileline), "%s:%d",file,line);
> >      fileline[sizeof(fileline)-1] = 0;
> >  
> > +    domain[0] = 0;
> > +    if (domid != INVALID_DOMID) snprintf(domain, sizeof(domain), "Domain 
> > %"PRIu32":", domid);
> 
> Coding style issue, please put snprintf in a new line.

I've mimicked what is a few lines above (see first line of the hunk context).
I agree it doesn't look too pretty. but then may be I should reformat the one 
above ;)

> > +    domain[sizeof(domain)-1] = 0;
> 
> This probably does do what you want it to do - sizeof(domain) -> 256.  I
> don't think you need this at all because IIRC snprintf puts a \0 at the
> end.

Oh indeed. I'll remove it then.

> Overall I think this is in line with what we discussed before.

I expected to send a v2 anyway, I'll resend with the changes.
--
Cedric

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