Re: [PATCH 058/104] virtiofsd: print log only when priority is high enough

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jan 06, 2020 at 05:05:11PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Thu, Dec 12, 2019 at 04:38:18PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: Eryu Guan <eguan@linux.alibaba.com>
> > > 
> > > Introduce "-o log_level=" command line option to specify current log
> > > level (priority), valid values are "debug info warn err", e.g.
> > > 
> > >     ./virtiofsd -o log_level=debug ...
> > > 
> > > So only log priority higher than "debug" will be printed to
> > > stderr/syslog. And the default level is info.
> > > 
> > > The "-o debug"/"-d" options are kept, and imply debug log level.
> > > 
> > > Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
> > > dgilbert: Reworked for libfuse's log_func
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  tools/virtiofsd/fuse_log.c       |   4 ++
> > >  tools/virtiofsd/fuse_lowlevel.c  |  75 ++++++++------------
> > >  tools/virtiofsd/fuse_lowlevel.h  |   1 +
> > >  tools/virtiofsd/helper.c         |  10 ++-
> > >  tools/virtiofsd/passthrough_ll.c | 118 +++++++++++++------------------
> > >  5 files changed, 92 insertions(+), 116 deletions(-)
> > 
> > > diff --git a/tools/virtiofsd/fuse_log.c b/tools/virtiofsd/fuse_log.c
> > > index 11345f9ec8..79a18a7aaa 100644
> > > --- a/tools/virtiofsd/fuse_log.c
> > > +++ b/tools/virtiofsd/fuse_log.c
> > > @@ -8,6 +8,10 @@
> > >   * See the file COPYING.LIB
> > >   */
> > >  
> > > +#include <stdbool.h>
> > > +#include <stdio.h>
> > > +#include <stdarg.h>
> > > +#include <syslog.h>
> > >  #include "fuse_log.h"
> > >  
> > >  #include <stdarg.h>
> > 
> > Why do we need to add these headers if there are no code changes in this
> > file ?
> 
> Thanks, those are left overs from an earlier version; I've deleted them now.
> 
> > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > > index f3c8bdf7cb..0abb369b3d 100644
> > > --- a/tools/virtiofsd/fuse_lowlevel.c
> > > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > > @@ -158,19 +158,17 @@ static int fuse_send_msg(struct fuse_session *se, struct fuse_chan *ch,
> > >      struct fuse_out_header *out = iov[0].iov_base;
> > >  
> > >      out->len = iov_length(iov, count);
> > > -    if (se->debug) {
> > > -        if (out->unique == 0) {
> > > -            fuse_log(FUSE_LOG_DEBUG, "NOTIFY: code=%d length=%u\n", out->error,
> > > -                     out->len);
> > > -        } else if (out->error) {
> > > -            fuse_log(FUSE_LOG_DEBUG,
> > > -                     "   unique: %llu, error: %i (%s), outsize: %i\n",
> > > -                     (unsigned long long)out->unique, out->error,
> > > -                     strerror(-out->error), out->len);
> > > -        } else {
> > > -            fuse_log(FUSE_LOG_DEBUG, "   unique: %llu, success, outsize: %i\n",
> > > -                     (unsigned long long)out->unique, out->len);
> > > -        }
> > > +    if (out->unique == 0) {
> > > +        fuse_log(FUSE_LOG_DEBUG, "NOTIFY: code=%d length=%u\n", out->error,
> > > +                 out->len);
> > > +    } else if (out->error) {
> > > +        fuse_log(FUSE_LOG_DEBUG,
> > > +                 "   unique: %llu, error: %i (%s), outsize: %i\n",
> > > +                 (unsigned long long)out->unique, out->error,
> > > +                 strerror(-out->error), out->len);
> > > +    } else {
> > > +        fuse_log(FUSE_LOG_DEBUG, "   unique: %llu, success, outsize: %i\n",
> > > +                 (unsigned long long)out->unique, out->len);
> > >      }
> > 
> > Removing all the 'if (se->debug)' checks means that we take the
> > performance hit of calling many logging functions in the common
> > case where debug is disabled. Hopefully 'fuse_log' is smart
> > enough to avoid printf formatting of the msg + args unless
> > it is actually goiing to output the message
> 
> It is; we go through fuse_log (fuse_log.c an imported file) that just
> does the va_start and then calls the log_func that was set later in this
> patch and the first thing it does is check the level and exit.

ok then

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|