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

On Thu, Sep 15, 2016 at 4:17 PM, Peter Krempa <pkrempa(a)redhat.com&gt; 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(a)redhat.com&gt; >> -- >> 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