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

2016-09-20 Thread Prasanna Kalever 
On Tue, Sep 20, 2016 at 3:36 PM, Peter Krempa  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 
>> ---
>>  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, ) < 0)
>> +if (qemuBuildDriveSourceStr(disk, , 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

--
libvir-list mailing list
libvir-list@redhat.com

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

2016-09-20 Thread Peter Krempa 
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 
> ---
>  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.

>  );
>  
>  
> @@ -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.

> +
>  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.

> +
>  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, ) < 0)
> +if (qemuBuildDriveSourceStr(disk, , 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list