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

[Prasanna Kalever]

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

else, please suggest.

Thanks for the review
--
Prasanna