[libvirt] [PATCH v1 1/1] qemu/gluster: add option for tuning debug logging level

[Daniel P. Berrange]

On Thu, Sep 15, 2016 at 06:09:02PM +0530, Prasanna Kalever wrote:
> On Thu, Sep 15, 2016 at 4:17 PM, Peter Krempa <pkrempa at redhat.com> wrote:
> > On Thu, Sep 15, 2016 at 15:32:48 +0530, Prasanna Kumar Kalever wrote:
> >> This helps in selecting log level of the gluster gfapi, output to stderr.
> >> The option is 'qemu_gfapi_debuglevel', can be tuned by editing
> >> '/etc/libvirt/qemu.conf'
> >>
> >> Debug levels ranges 0-9, with 9 being the most verbose, and 0 representing
> >> no debugging output.  The default is the same as it was before, which
> >> is a level of 4.  The current logging levels defined in the gluster
> >> gfapi are:
> >>
> >>     0 - None
> >>     1 - Emergency
> >>     2 - Alert
> >>     3 - Critical
> >>     4 - Error
> >>     5 - Warning
> >>     6 - Notice
> >>     7 - Info
> >>     8 - Debug
> >>     9 - Trace
> >>
> >> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever at redhat.com>
> >> --
> >> v1: Initial post
> >> ---
> >>  src/qemu/qemu.conf        | 20 ++++++++++++++++++++
> >>  src/qemu/qemu_command.c   | 12 +++++++++++-
> >>  src/qemu/qemu_conf.c      |  3 +++
> >>  src/qemu/qemu_conf.h      |  1 +
> >>  src/util/virstoragefile.h |  2 ++
> >>  5 files changed, 37 insertions(+), 1 deletion(-)
> >
> > This patch fails qemuxml2argvtest and make syntax-check. Please make
> > sure to run the testsuite and syntax-check before submission.
> 
> Apologies for ignoring the guidelines.
> 
> >
> >>
> >> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> >> index e4c2aae..c6c8f3a 100644
> >> --- a/src/qemu/qemu.conf
> >> +++ b/src/qemu/qemu.conf
> >> @@ -621,3 +621,23 @@
> >>  #          rollover when a size limit is hit.
> >>  #
> >>  #stdio_handler = "logd"
> >> +
> >> +# Qemu gluster libgfapi log level, debug levels are 0-9, with 9 being the
> >> +# most verbose, and 0 representing no debugging output.
> >> +#
> >> +# The current logging levels defined in the gluster GFAPI are:
> >> +#
> >> +#    0 - None
> >> +#    1 - Emergency
> >> +#    2 - Alert
> >> +#    3 - Critical
> >> +#    4 - Error
> >> +#    5 - Warning
> >> +#    6 - Notice
> >> +#    7 - Info
> >> +#    8 - Debug
> >> +#    9 - Trace
> >> +#
> >> +# Defaults to 4
> >> +#
> >> +# qemu_gfapi_debuglevel = 9
> >
> > Missing change to src/qemu/libvirtd_qemu.aug
> 
> Peter, can you please teach me what is the essence of this ?
> 
> >
> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >> index 3a61863..650eedc 100644
> >> --- a/src/qemu/qemu_command.c
> >> +++ b/src/qemu/qemu_command.c
> >> @@ -859,6 +859,7 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
> >>       /* { driver:"gluster",
> >>        *   volume:"testvol",
> >>        *   path:"/a.img",
> >> +      *   debug:9,
> >>        *   server :[{type:"tcp", host:"1.2.3.4", port:24007},
> >>        *            {type:"unix", socket:"/tmp/glusterd.socket"}, ...]}
> >>        */
> >> @@ -866,6 +867,7 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
> >>                                   "s:driver", protocol,
> >>                                   "s:volume", src->volume,
> >>                                   "s:path", src->path,
> >> +                                 "u:debug", src->debug_level,
> >>                                   "a:server", servers, NULL) < 0)
> >>            virJSONValueFree(servers);
> >>
> >> @@ -2177,6 +2179,7 @@ qemuBuildDriveDevStr(const virDomainDef *def,
> >>
> >>  static int
> >>  qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> >> +                              virQEMUDriverConfigPtr cfg,
> >>                                const virDomainDef *def,
> >>                                virQEMUCapsPtr qemuCaps)
> >>  {
> >> @@ -2255,6 +2258,13 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> >>
> >>          virCommandAddArg(cmd, "-drive");
> >>
> >> +        if (disk->src &&
> >> +                disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
> >
> > Misaligned.
> >
> >> +            if(cfg->qemuGfapiDebugLevel) {
> >> +                disk->src->debug_level = cfg->qemuGfapiDebugLevel;
> >> +            }
> >
> > Fails at least two syntax-check rules.
> >
> >> +        }
> >
> > Since this is done for gluster only I don't see a reason to have a
> > debug_level property for the storage source. It would make sense if you
> > could configure it on a per disk basis.
> 
> I understand that the concern is w.r.t adding debug_level in storage
> source i.e. in virStorageSourcePtr
> 
> I'm not sure if I'm following the suggested solution here. Can you
> please elaborate more.
> 
> >
> >> +
> >>          if (!(optstr = qemuBuildDriveStr(disk, driveBoot, qemuCaps)))
> >>              return -1;
> >>          virCommandAddArg(cmd, optstr);
> >
> > Peter
> 
> 
> Not sure if you have noticed it, I have also ignored the URI part of
> gluster protocol.
> Along with the comments, I shall also address that part URI part in
> the next version of this patch.
> 
> Peter let me know if its worth to split the patches in below fashion.
> 
> 1/4: URI changes
> 2/4: test case changes
> 3/4: JSON changes
> 4/4: test case changes for JSON

Splitting it is overkill IMHO, as the changes are all logically part
of the same feature.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|