[libvirt] [PATCH 2/2] qemu: Fix DEATHCODE error in qemuDomainGetBlockIoTune.

2014-11-12 Thread Matthias Gatto 
reported here: 
http://www.redhat.com/archives/libvir-list/2014-November/msg00327.html

I could have just remove bool supportMaxOptions variable, but
if I had do this, we could not check anymore if the nparams variable is
superior to QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX.

Signed-off-by: Matthias Gatto matthias.ga...@outscale.com
---
 src/qemu/qemu_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 56e8430..61c4af6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17024,6 +17024,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
 
 if (flags  VIR_DOMAIN_AFFECT_LIVE) {
 priv = vm-privateData;
+supportMaxOptions = virQEMUCapsGet(priv-qemuCaps, 
QEMU_CAPS_DRIVE_IOTUNE_MAX);
 qemuDomainObjEnterMonitor(driver, vm);
 ret = qemuMonitorGetBlockIoThrottle(priv-mon, device, reply, 
supportMaxOptions);
 qemuDomainObjExitMonitor(driver, vm);
-- 
1.8.3.1

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

Re: [libvirt] [PATCH 2/2] qemu: Fix DEATHCODE error in qemuDomainGetBlockIoTune.

2014-11-12 Thread John Ferlan 

Need to fix the commit message to something like Resolve Coverity DEADCODE  
(something I just realized for 1/2 which could read Resolve Coverity 
COPY_PASTE_ERROR

Something I can take care of when pushing.

On 11/12/2014 08:04 AM, Matthias Gatto wrote:
 reported here: 
 http://www.redhat.com/archives/libvir-list/2014-November/msg00327.html
 
 I could have just remove bool supportMaxOptions variable, but
 if I had do this, we could not check anymore if the nparams variable is
 superior to QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX.
 
 Signed-off-by: Matthias Gatto matthias.ga...@outscale.com
 ---
  src/qemu/qemu_driver.c | 1 +
  1 file changed, 1 insertion(+)
 

While the following does work...

 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 56e8430..61c4af6 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -17024,6 +17024,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
  
  if (flags  VIR_DOMAIN_AFFECT_LIVE) {
  priv = vm-privateData;
 +supportMaxOptions = virQEMUCapsGet(priv-qemuCaps, 
 QEMU_CAPS_DRIVE_IOTUNE_MAX);
  qemuDomainObjEnterMonitor(driver, vm);
  ret = qemuMonitorGetBlockIoThrottle(priv-mon, device, reply, 
 supportMaxOptions);
  qemuDomainObjExitMonitor(driver, vm);
 

Would perhaps the following be cleaner:

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 56e8430..5124e56 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17003,14 +17003,15 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
 persistentDef)  0)
 goto endjob;
 
+/* If the VM is running, we can check if the current VM can use
+ * optional parameters or not. We didn't made this check sooner
+ * because we need the VM data to do so. */
+priv = vm-privateData;
+if (flags  VIR_DOMAIN_AFFECT_LIVE)
+supportMaxOptions = virQEMUCapsGet(priv-qemuCaps,
+   QEMU_CAPS_DRIVE_IOTUNE_MAX);
+
 if ((*nparams) == 0) {
-if (flags  VIR_DOMAIN_AFFECT_LIVE) {
-priv = vm-privateData;
-/* If the VM is running, we can check if the current VM can use
- * optional parameters or not. We didn't made this check sooner
- * because we need the VM data to do so. */
-supportMaxOptions = virQEMUCapsGet(priv-qemuCaps, 
QEMU_CAPS_DRIVE_IOTUNE_MAX);
-}
 *nparams = supportMaxOptions ?
QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX : 
QEMU_NB_BLOCK_IO_TUNE_PARAM;
 ret = 0;
@@ -17023,7 +17024,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
 }
 
 if (flags  VIR_DOMAIN_AFFECT_LIVE) {
-priv = vm-privateData;
 qemuDomainObjEnterMonitor(driver, vm);
 ret = qemuMonitorGetBlockIoThrottle(priv-mon, device, reply, 
supportMaxOptions);
 qemuDomainObjExitMonitor(driver, vm);


The difference being moving the setting of 'priv' to be made
regardless of flags and supportMaxOptions only once prior to
using in either place. The second sentence of your comment is
a bit confusing too - I assume the VM data you are referencing
is the vm obtained much earlier.  Or are you trying to indicate
that perhaps one of the interceding calls is necessary?

In any case, does the above look reasonable?

Tks - 

John

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

Re: [libvirt] [PATCH 2/2] qemu: Fix DEATHCODE error in qemuDomainGetBlockIoTune.

2014-11-12 Thread Matthias Gatto 
On Wed, Nov 12, 2014 at 1:27 PM, John Ferlan jfer...@redhat.com wrote:

 Need to fix the commit message to something like Resolve Coverity DEADCODE  
 (something I just realized for 1/2 which could read Resolve Coverity 
 COPY_PASTE_ERROR

 Something I can take care of when pushing.

 On 11/12/2014 08:04 AM, Matthias Gatto wrote:
 reported here: 
 http://www.redhat.com/archives/libvir-list/2014-November/msg00327.html

 I could have just remove bool supportMaxOptions variable, but
 if I had do this, we could not check anymore if the nparams variable is
 superior to QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX.

 Signed-off-by: Matthias Gatto matthias.ga...@outscale.com
 ---
  src/qemu/qemu_driver.c | 1 +
  1 file changed, 1 insertion(+)


 While the following does work...

 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 56e8430..61c4af6 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -17024,6 +17024,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,

  if (flags  VIR_DOMAIN_AFFECT_LIVE) {
  priv = vm-privateData;
 +supportMaxOptions = virQEMUCapsGet(priv-qemuCaps, 
 QEMU_CAPS_DRIVE_IOTUNE_MAX);
  qemuDomainObjEnterMonitor(driver, vm);
  ret = qemuMonitorGetBlockIoThrottle(priv-mon, device, reply, 
 supportMaxOptions);
  qemuDomainObjExitMonitor(driver, vm);


 Would perhaps the following be cleaner:

 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 56e8430..5124e56 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -17003,14 +17003,15 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
  persistentDef)  0)
  goto endjob;

 +/* If the VM is running, we can check if the current VM can use
 + * optional parameters or not. We didn't made this check sooner
 + * because we need the VM data to do so. */
 +priv = vm-privateData;
 +if (flags  VIR_DOMAIN_AFFECT_LIVE)
 +supportMaxOptions = virQEMUCapsGet(priv-qemuCaps,
 +   QEMU_CAPS_DRIVE_IOTUNE_MAX);
 +
  if ((*nparams) == 0) {
 -if (flags  VIR_DOMAIN_AFFECT_LIVE) {
 -priv = vm-privateData;
 -/* If the VM is running, we can check if the current VM can use
 - * optional parameters or not. We didn't made this check sooner
 - * because we need the VM data to do so. */
 -supportMaxOptions = virQEMUCapsGet(priv-qemuCaps, 
 QEMU_CAPS_DRIVE_IOTUNE_MAX);
 -}
  *nparams = supportMaxOptions ?
 QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX : 
 QEMU_NB_BLOCK_IO_TUNE_PARAM;
  ret = 0;
 @@ -17023,7 +17024,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
  }

  if (flags  VIR_DOMAIN_AFFECT_LIVE) {
 -priv = vm-privateData;
  qemuDomainObjEnterMonitor(driver, vm);
  ret = qemuMonitorGetBlockIoThrottle(priv-mon, device, reply, 
 supportMaxOptions);
  qemuDomainObjExitMonitor(driver, vm);


 The difference being moving the setting of 'priv' to be made
 regardless of flags and supportMaxOptions only once prior to
 using in either place. The second sentence of your comment is
 a bit confusing too - I assume the VM data you are referencing
 is the vm obtained much earlier.  Or are you trying to indicate
 that perhaps one of the interceding calls is necessary?

 In any case, does the above look reasonable?

 Tks -

 John

Actually it's both: i need  vm-privateData; to use virQEMUCapsGet but
vm-privateData need qemuDomainObjBeginJob and virDomainLiveConfigHelperMethod.

Your proposal is effectively cleaner, but I don't think we need to set
priv = vm-privateData if the VM is not running, so i've made this:

+if (flags  VIR_DOMAIN_AFFECT_LIVE) {
+/* If the VM is running, we can check if the current VM can
use
+ * optional parameters or not. We didn't made this check
sooner
+ * because we need the VM data to do so. */
+priv = vm-privateData;
+supportMaxOptions = virQEMUCapsGet(priv-qemuCaps,
QEMU_CAPS_DRIVE_IOTUNE_MAX);
+}
+
 if ((*nparams) == 0) {
-if (flags  VIR_DOMAIN_AFFECT_LIVE) {
-priv = vm-privateData;
-/* If the VM is running, we can check if the current VM
can use
- * optional parameters or not. We didn't made this check
sooner
- * because we need the VM data to do so. */
-supportMaxOptions = virQEMUCapsGet(priv-qemuCaps,
QEMU_CAPS_DRIVE_IOTUNE_MAX);
-}
 *nparams = supportMaxOptions ?
QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX :
QEMU_NB_BLOCK_IO_TUNE_PARAM;
@@ -17023,7 +17024,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
 }

 if (flags  VIR_DOMAIN_AFFECT_LIVE) {
-priv = vm-privateData;
 qemuDomainObjEnterMonitor(driver, vm);
 ret = qemuMonitorGetBlockIoThrottle(priv-mon, device,
reply, supportMaxOptions);
 qemuDomainObjExitMonitor(driver, vm);