[libvirt] [PATCH v3 2/2] qemu_capabilities: Introduce gluster specific debug capability

[Prasanna Kalever]

On Tue, Sep 20, 2016 at 3:36 PM, Peter Krempa <pkrempa at redhat.com> wrote:
> On Tue, Sep 20, 2016 at 15:23:04 +0530, Prasanna Kumar Kalever wrote:
>> Teach qemu driver to detect whether qemu supports this configuration
>> factor or not.
>>
>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever at redhat.com>
>> ---
>>  src/qemu/qemu_capabilities.c |  6 ++++++
>>  src/qemu/qemu_capabilities.h |  3 +++
>>  src/qemu/qemu_command.c      | 13 ++++++++-----
>>  tests/qemuxml2argvtest.c     |  3 ++-
>>  4 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index c71b4dc..c4350d5 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -344,6 +344,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>                "query-hotpluggable-cpus",
>>
>>                "virtio-net.rx_queue_size", /* 235 */
>> +
>> +              "debug",  /* 236 */
>
> This is a VERY bad name for a feature flag. Also please follow the
> pattern in the file. It's 5 entries per block, very easy to spot.

Peter, I'm pretty bad at naming something, can either of you or Daniel
can please suggest one, that will be great.

Apologies, 5 block entry just missed my eyes or I might have overlooked at it.

>
>>      );
>>
>>
>> @@ -2496,6 +2498,10 @@ virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps,
>>          qemuMonitorSupportsActiveCommit(mon))
>>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_ACTIVE_COMMIT);
>>
>> +    /* Since qemu 2.7.0 */
>> +    if (qemuCaps->version >= 2007000)
>> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL);
>
> Version checks are bad. Is there anything you can base this on that can
> be detected? Version checks break with backports.

I don't think we have a better choice w.r.t checks ?

>
>> +
>>      return 0;
>>  }
>>
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index 26ac1fa..ac9f4c8 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -379,6 +379,9 @@ typedef enum {
>>      /* 235 */
>>      QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, /* virtio-net-*.rx_queue_size */
>>
>> +    /* 236 */
>> +    QEMU_CAPS_GLUSTER_DEBUG_LEVEL, /* -drive debug={0..9} */
>
> You are breaking the pattern.

Yeah, thank you!

>
>> +
>>      QEMU_CAPS_LAST /* this must always be the last item */
>>  } virQEMUCapsFlags;
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 545e25d..ad0f1b2 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1314,7 +1314,8 @@ qemuDiskBusNeedsDeviceArg(int bus)
>>
>>  static int
>>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>> -                        virBufferPtr buf)
>> +                        virBufferPtr buf,
>> +                  virQEMUCapsPtr qemuCaps)
>
> Misaligned.
>
>>  {
>>      int actualType = virStorageSourceGetActualType(disk->src);
>>      qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>> @@ -1385,7 +1386,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>
>>      if (disk->src &&
>>          disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
>> -        virBufferAsprintf(buf, "file.debug=%d,", disk->src->debug_level);
>> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL))
>> +            virBufferAsprintf(buf, "file.debug=%d,", disk->src->debug_level);
>>      }
>>
>>      if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>> @@ -1513,7 +1515,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>>          break;
>>      }
>>
>> -    if (qemuBuildDriveSourceStr(disk, &opt) < 0)
>> +    if (qemuBuildDriveSourceStr(disk, &opt, qemuCaps) < 0)
>>          goto error;
>>
>>      if (emitDeviceSyntax)
>> @@ -2263,8 +2265,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>>
>>          if (disk->src &&
>>              disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
>> -            if (cfg->glusterDebugLevel)
>> -                disk->src->debug_level = cfg->glusterDebugLevel;
>> +            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL) &&
>> +                cfg->glusterDebugLevel)
>> +                    disk->src->debug_level = cfg->glusterDebugLevel;
>>          }
>>
>>          if (!(optstr = qemuBuildDriveStr(disk, driveBoot, qemuCaps)))
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index dbb0e4d..b29a63b 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -832,7 +832,8 @@ mymain(void)
>>      DO_TEST("disk-drive-network-iscsi-lun",
>>              QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI,
>>              QEMU_CAPS_SCSI_BLOCK);
>> -    DO_TEST("disk-drive-network-gluster", NONE);
>> +    DO_TEST("disk-drive-network-gluster",
>> +            QEMU_CAPS_GLUSTER_DEBUG_LEVEL);
>
> This patch fails make check. You forgot to bundle in some changes.
>
> Peter