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

[Prasanna Kalever]

On Wed, Sep 21, 2016 at 7:28 PM, Jiri Denemark <jdenemar at redhat.com> wrote:
> On Wed, Sep 21, 2016 at 19:00:16 +0530, Prasanna Kumar Kalever wrote:
>> This helps in selecting log level of the gluster gfapi, output to stderr.
>> The option is 'gluster_debug_level', 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>
> ...
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 3a61863..545e25d 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1383,6 +1383,11 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>      }
>>      virBufferAddLit(buf, ",");
>>
>> +    if (disk->src &&
>> +        disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
>> +        virBufferAsprintf(buf, "file.debug=%d,", disk->src->debug_level);
>
> This should be only done if QEMU supports it; see my comment to patch 2.

Addressed in patch 2/2

>
> And just use cfg->glusterDebugLevel directly here instead of passing it
> via disk->src->debug_level.

Just felt it will be too much routing of cfg to all the way in the
function stack just for the sake of glusterDebugLevel.
Hence I went with storing it in storage structure.

>
>> +    }
>> +
>>      if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>>          /* NB: If libvirt starts using the more modern option based
>>           *     syntax to build the command line (e.g., "-drive driver=rbd,
>> @@ -2177,6 +2182,7 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>>
>>  static int
>>  qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>> +                              virQEMUDriverConfigPtr cfg,
>>                                const virDomainDef *def,
>>                                virQEMUCapsPtr qemuCaps)
>>  {
>> @@ -2255,6 +2261,12 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>>
>>          virCommandAddArg(cmd, "-drive");
>>
>> +        if (disk->src &&
>> +            disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
>> +            if (cfg->glusterDebugLevel)
>> +                disk->src->debug_level = cfg->glusterDebugLevel;
>> +        }
>> +
>>          if (!(optstr = qemuBuildDriveStr(disk, driveBoot, qemuCaps)))
>>              return -1;
>>          virCommandAddArg(cmd, optstr);
>
> Please, don't store the static debug level in disk->src. NACK to this
> hunk.

Since Peter also pointed about the same in the initial patches, let me
pass cfg to all the callee's. Never Mind.

>
> ...
>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>> index 3d09468..9f3add3 100644
>> --- a/src/util/virstoragefile.h
>> +++ b/src/util/virstoragefile.h
>> @@ -237,6 +237,8 @@ struct _virStorageSource {
>>      virStorageAuthDefPtr auth;
>>      virStorageEncryptionPtr encryption;
>>
>> +    unsigned int debug_level;
>> +
>>      char *driverName;
>>      int format; /* virStorageFileFormat in domain backing chains, but
>>                   * pool-specific enum for storage volumes */
>
> And NACK to this hunk, too.

taken care when using/passing cfg.

--
Prasanna

> ...
>
> Jirka