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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> --
> >> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list