Re: [libvirt] [PATCH v4 2/2] qemu_capabilities: Introduce gluster specific debug capability
On Wed, Sep 21, 2016 at 7:21 PM, Jiri Denemark <jdenemar(a)redhat.com> wrote:
On Wed, Sep 21, 2016 at 19:00:17 +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(a)redhat.com>
...i
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 545e25d..b73ad70 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)
> {
> 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);
Adding file.debug unconditionally first and making it conditional in the
next patch is pretty strange. The capability should really be introduced
as the first patch in the series to avoid it.
This is something I have learned form the log/history.
As long as these are in series, I don't think that really matters ?
Also the cover letter prepares the reviewers for it, do let me know if
it is unclear there.
In case, if you think this is blocker/stopping this patch, let me know
I shall squash them up.
Thanks Jirka!
--
Prasanna
> }
>
> 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;
Wrong indentation, but it doesn't matter much because of my comment on
this code in patch 1.
...
Jirka