11:45 a.m.
Both are false positive but its nice to make coverity happy.
Pavel Hrdina (2):
vbox_storage: fix coverity issue with overwriting value
hotplug: fix char device detach
src/qemu/qemu_hotplug.c | 12 ++++++++----
src/vbox/vbox_storage.c | 36 ++++++++++++++++--------------------
2 files changed, 24 insertions(+), 24 deletions(-)
--
2.0.4
11:45 a.m.
New subject: [libvirt] [PATCH 1/2] vbox_storage: fix coverity issue with overwriting value
Coverity is complaining about overwriting value in 'rc' variable
without using the old value because it somehow doesn't recognize that
the value is used by MACRO. The 'rc' variable is there only for checking
return code so it's save to remove it and make coverity happy.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/vbox/vbox_storage.c | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c
index 3610a35..0cf7a33 100644
--- a/src/vbox/vbox_storage.c
+++ b/src/vbox/vbox_storage.c
@@ -551,7 +551,6 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
PRUint32 machineIdsSize = 0;
vboxArray machineIds = VBOX_ARRAY_INITIALIZER;
vboxIIDUnion hddIID;
- nsresult rc;
int ret = -1;
if (!data->vboxObj) {
@@ -568,8 +567,9 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
}
vboxIIDFromUUID(&hddIID, uuid);
- rc = gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj, &hddIID,
&hardDisk);
- if (NS_FAILED(rc))
+ if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj,
+ &hddIID,
+ &hardDisk)))
goto cleanup;
gVBoxAPI.UIMedium.GetState(hardDisk, &hddstate);
@@ -603,23 +603,22 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
vboxIIDFromArrayItem(&machineId, &machineIds, i);
if (gVBoxAPI.getMachineForSession) {
- rc = gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj, &machineId,
&machine);
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj,
+ &machineId,
+ &machine))) {
virReportError(VIR_ERR_NO_DOMAIN, "%s",
_("no domain with matching uuid"));
break;
}
}
- rc = gVBoxAPI.UISession.Open(data, &machineId, machine);
-
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(gVBoxAPI.UISession.Open(data, &machineId, machine))) {
vboxIIDUnalloc(&machineId);
continue;
}
- rc = gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
- if (NS_FAILED(rc))
+ if (NS_FAILED(gVBoxAPI.UISession.GetMachine(data->vboxSession,
+ &machine)))
goto cleanupLoop;
gVBoxAPI.UArray.vboxArrayGet(&hddAttachments, machine,
@@ -633,13 +632,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
if (!hddAttachment)
continue;
- rc = gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment, &hdd);
- if (NS_FAILED(rc) || !hdd)
+ if (NS_FAILED(gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment,
+ &hdd)) || !hdd)
continue;
VBOX_IID_INITIALIZE(&iid);
- rc = gVBoxAPI.UIMedium.GetId(hdd, &iid);
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(gVBoxAPI.UIMedium.GetId(hdd, &iid))) {
VBOX_MEDIUM_RELEASE(hdd);
continue;
}
@@ -658,9 +656,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
gVBoxAPI.UIMediumAttachment.GetPort(hddAttachment, &port);
gVBoxAPI.UIMediumAttachment.GetDevice(hddAttachment, &device);
- rc = gVBoxAPI.UIMachine.DetachDevice(machine, controller, port, device);
- if (NS_SUCCEEDED(rc)) {
- rc = gVBoxAPI.UIMachine.SaveSettings(machine);
+ if (NS_SUCCEEDED(gVBoxAPI.UIMachine.DetachDevice(machine, controller,
port, device))) {
+ ignore_value(gVBoxAPI.UIMachine.SaveSettings(machine));
VIR_DEBUG("saving machine settings");
deregister++;
VIR_DEBUG("deregistering hdd:%d", deregister);
@@ -683,9 +680,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
if (machineIdsSize == 0 || machineIdsSize == deregister) {
IProgress *progress = NULL;
- rc = gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress);
-
- if (NS_SUCCEEDED(rc) && progress) {
+ if (NS_SUCCEEDED(gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress))
&&
+ progress) {
gVBoxAPI.UIProgress.WaitForCompletion(progress, -1);
VBOX_RELEASE(progress);
DEBUGIID("HardDisk deleted, UUID", &hddIID);
--
2.0.4
12:05 p.m.
New subject: [libvirt] [PATCH 1/2] vbox_storage: fix coverity issue with overwriting value
On 10/31/2014 12:45 PM, Pavel Hrdina wrote:
Coverity is complaining about overwriting value in 'rc'
variable
without using the old value because it somehow doesn't recognize that
the value is used by MACRO. The 'rc' variable is there only for checking
return code so it's save to remove it and make coverity happy.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/vbox/vbox_storage.c | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c
index 3610a35..0cf7a33 100644
--- a/src/vbox/vbox_storage.c
+++ b/src/vbox/vbox_storage.c
@@ -551,7 +551,6 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
PRUint32 machineIdsSize = 0;
vboxArray machineIds = VBOX_ARRAY_INITIALIZER;
vboxIIDUnion hddIID;
- nsresult rc;
int ret = -1;
if (!data->vboxObj) {
@@ -568,8 +567,9 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
}
vboxIIDFromUUID(&hddIID, uuid);
- rc = gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj, &hddIID,
&hardDisk);
- if (NS_FAILED(rc))
+ if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj,
+ &hddIID,
+ &hardDisk)))
goto cleanup;
gVBoxAPI.UIMedium.GetState(hardDisk, &hddstate);
@@ -603,23 +603,22 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
vboxIIDFromArrayItem(&machineId, &machineIds, i);
if (gVBoxAPI.getMachineForSession) {
- rc = gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj, &machineId,
&machine);
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj,
+ &machineId,
+ &machine))) {
virReportError(VIR_ERR_NO_DOMAIN, "%s",
_("no domain with matching uuid"));
break;
}
}
- rc = gVBoxAPI.UISession.Open(data, &machineId, machine);
-
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(gVBoxAPI.UISession.Open(data, &machineId, machine))) {
vboxIIDUnalloc(&machineId);
continue;
}
- rc = gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
- if (NS_FAILED(rc))
+ if (NS_FAILED(gVBoxAPI.UISession.GetMachine(data->vboxSession,
+ &machine)))
goto cleanupLoop;
gVBoxAPI.UArray.vboxArrayGet(&hddAttachments, machine,
@@ -633,13 +632,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
if (!hddAttachment)
continue;
- rc = gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment, &hdd);
- if (NS_FAILED(rc) || !hdd)
+ if (NS_FAILED(gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment,
+ &hdd)) || !hdd)
continue;
VBOX_IID_INITIALIZE(&iid);
- rc = gVBoxAPI.UIMedium.GetId(hdd, &iid);
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(gVBoxAPI.UIMedium.GetId(hdd, &iid))) {
VBOX_MEDIUM_RELEASE(hdd);
continue;
}
@@ -658,9 +656,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
gVBoxAPI.UIMediumAttachment.GetPort(hddAttachment, &port);
gVBoxAPI.UIMediumAttachment.GetDevice(hddAttachment, &device);
- rc = gVBoxAPI.UIMachine.DetachDevice(machine, controller, port,
device);
- if (NS_SUCCEEDED(rc)) {
- rc = gVBoxAPI.UIMachine.SaveSettings(machine);
+ if (NS_SUCCEEDED(gVBoxAPI.UIMachine.DetachDevice(machine, controller,
port, device))) {
+ ignore_value(gVBoxAPI.UIMachine.SaveSettings(machine));
While this does work (eg, the ignore_value), is that the intention?
I asked the author :
http://www.redhat.com/archives/libvir-list/2014-October/msg01050.html
if the intention was to ignore the 'rc' value or not? The question
being of course if the SaveSettings fails, should the rest of the code
be executed? I don't know what to expect in vBox..
As painful as it is with the daily failing Coverity tests - I was hoping
we could get an answer there first.
John
VIR_DEBUG("saving machine settings");
deregister++;
VIR_DEBUG("deregistering hdd:%d", deregister);
@@ -683,9 +680,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int
flags)
if (machineIdsSize == 0 || machineIdsSize == deregister) {
IProgress *progress = NULL;
- rc = gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress);
-
- if (NS_SUCCEEDED(rc) && progress) {
+ if (NS_SUCCEEDED(gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress))
&&
+ progress) {
gVBoxAPI.UIProgress.WaitForCompletion(progress, -1);
VBOX_RELEASE(progress);
DEBUGIID("HardDisk deleted, UUID", &hddIID);
12:14 p.m.
New subject: [libvirt] [PATCH 1/2] vbox_storage: fix coverity issue with overwriting value
On 10/31/2014 06:05 PM, John Ferlan wrote:
On 10/31/2014 12:45 PM, Pavel Hrdina wrote:
> Coverity is complaining about overwriting value in 'rc' variable
> without using the old value because it somehow doesn't recognize that
> the value is used by MACRO. The 'rc' variable is there only for checking
> return code so it's save to remove it and make coverity happy.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/vbox/vbox_storage.c | 36 ++++++++++++++++--------------------
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c
> index 3610a35..0cf7a33 100644
> --- a/src/vbox/vbox_storage.c
> +++ b/src/vbox/vbox_storage.c
> @@ -551,7 +551,6 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned
int flags)
> PRUint32 machineIdsSize = 0;
> vboxArray machineIds = VBOX_ARRAY_INITIALIZER;
> vboxIIDUnion hddIID;
> - nsresult rc;
> int ret = -1;
>
> if (!data->vboxObj) {
> @@ -568,8 +567,9 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned
int flags)
> }
>
> vboxIIDFromUUID(&hddIID, uuid);
> - rc = gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj, &hddIID,
&hardDisk);
> - if (NS_FAILED(rc))
> + if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj,
> + &hddIID,
> + &hardDisk)))
> goto cleanup;
>
> gVBoxAPI.UIMedium.GetState(hardDisk, &hddstate);
> @@ -603,23 +603,22 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned
int flags)
> vboxIIDFromArrayItem(&machineId, &machineIds, i);
>
> if (gVBoxAPI.getMachineForSession) {
> - rc = gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj, &machineId,
&machine);
> - if (NS_FAILED(rc)) {
> + if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj,
> + &machineId,
> + &machine))) {
> virReportError(VIR_ERR_NO_DOMAIN, "%s",
> _("no domain with matching uuid"));
> break;
> }
> }
>
> - rc = gVBoxAPI.UISession.Open(data, &machineId, machine);
> -
> - if (NS_FAILED(rc)) {
> + if (NS_FAILED(gVBoxAPI.UISession.Open(data, &machineId, machine))) {
> vboxIIDUnalloc(&machineId);
> continue;
> }
>
> - rc = gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
> - if (NS_FAILED(rc))
> + if (NS_FAILED(gVBoxAPI.UISession.GetMachine(data->vboxSession,
> + &machine)))
> goto cleanupLoop;
>
> gVBoxAPI.UArray.vboxArrayGet(&hddAttachments, machine,
> @@ -633,13 +632,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned
int flags)
> if (!hddAttachment)
> continue;
>
> - rc = gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment, &hdd);
> - if (NS_FAILED(rc) || !hdd)
> + if (NS_FAILED(gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment,
> + &hdd)) || !hdd)
> continue;
>
> VBOX_IID_INITIALIZE(&iid);
> - rc = gVBoxAPI.UIMedium.GetId(hdd, &iid);
> - if (NS_FAILED(rc)) {
> + if (NS_FAILED(gVBoxAPI.UIMedium.GetId(hdd, &iid))) {
> VBOX_MEDIUM_RELEASE(hdd);
> continue;
> }
> @@ -658,9 +656,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned
int flags)
> gVBoxAPI.UIMediumAttachment.GetPort(hddAttachment, &port);
> gVBoxAPI.UIMediumAttachment.GetDevice(hddAttachment, &device);
>
> - rc = gVBoxAPI.UIMachine.DetachDevice(machine, controller, port,
device);
> - if (NS_SUCCEEDED(rc)) {
> - rc = gVBoxAPI.UIMachine.SaveSettings(machine);
> + if (NS_SUCCEEDED(gVBoxAPI.UIMachine.DetachDevice(machine,
controller, port, device))) {
> + ignore_value(gVBoxAPI.UIMachine.SaveSettings(machine));
While this does work (eg, the ignore_value), is that the intention?
I asked the author :
http://www.redhat.com/archives/libvir-list/2014-October/msg01050.html
if the intention was to ignore the 'rc' value or not? The question
being of course if the SaveSettings fails, should the rest of the code
be executed? I don't know what to expect in vBox..
As painful as it is with the daily failing Coverity tests - I was hoping
we could get an answer there first.
Yes I've noticed that email and it would be probably nice to get answer
to this question and also fix it if it should fail. If we get an answer
before release I can send a v2 or just update the code and push if there
is nothing else wrong with this patch. The reason I've sent those
patches now is to get a review for the resolution of coverity complains.
Pavel
John
> VIR_DEBUG("saving machine settings");
> deregister++;
> VIR_DEBUG("deregistering hdd:%d", deregister);
> @@ -683,9 +680,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, unsigned
int flags)
>
> if (machineIdsSize == 0 || machineIdsSize == deregister) {
> IProgress *progress = NULL;
> - rc = gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress);
> -
> - if (NS_SUCCEEDED(rc) && progress) {
> + if (NS_SUCCEEDED(gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress))
&&
> + progress) {
> gVBoxAPI.UIProgress.WaitForCompletion(progress, -1);
> VBOX_RELEASE(progress);
> DEBUGIID("HardDisk deleted, UUID", &hddIID);
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:46 p.m.
New subject: [libvirt] [PATCH 1/2] vbox_storage: fix coverity issue with overwriting value
On 10/31/2014 01:14 PM, Pavel Hrdina wrote:
On 10/31/2014 06:05 PM, John Ferlan wrote:
>
>
> On 10/31/2014 12:45 PM, Pavel Hrdina wrote:
>> Coverity is complaining about overwriting value in 'rc' variable
>> without using the old value because it somehow doesn't recognize that
>> the value is used by MACRO. The 'rc' variable is there only for checking
>> return code so it's save to remove it and make coverity happy.
>>
>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> ---
>> src/vbox/vbox_storage.c | 36 ++++++++++++++++--------------------
>> 1 file changed, 16 insertions(+), 20 deletions(-)
>>
In the event we don't get an answer - this does resolve the Coverity
issue, so to that aspect you have an ACK
Looking at other callers in vbox_common.c - it seems only one cares
whether it works or not, so the "norm" is to ignore the failure...
John
>> diff --git a/src/vbox/vbox_storage.c
b/src/vbox/vbox_storage.c
>> index 3610a35..0cf7a33 100644
>> --- a/src/vbox/vbox_storage.c
>> +++ b/src/vbox/vbox_storage.c
>> @@ -551,7 +551,6 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
unsigned int flags)
>> PRUint32 machineIdsSize = 0;
>> vboxArray machineIds = VBOX_ARRAY_INITIALIZER;
>> vboxIIDUnion hddIID;
>> - nsresult rc;
>> int ret = -1;
>>
>> if (!data->vboxObj) {
>> @@ -568,8 +567,9 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
unsigned int flags)
>> }
>>
>> vboxIIDFromUUID(&hddIID, uuid);
>> - rc = gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj, &hddIID,
&hardDisk);
>> - if (NS_FAILED(rc))
>> + if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj,
>> + &hddIID,
>> + &hardDisk)))
>> goto cleanup;
>>
>> gVBoxAPI.UIMedium.GetState(hardDisk, &hddstate);
>> @@ -603,23 +603,22 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
unsigned int flags)
>> vboxIIDFromArrayItem(&machineId, &machineIds, i);
>>
>> if (gVBoxAPI.getMachineForSession) {
>> - rc = gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj,
&machineId, &machine);
>> - if (NS_FAILED(rc)) {
>> + if (NS_FAILED(gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj,
>> + &machineId,
>> + &machine))) {
>> virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> _("no domain with matching uuid"));
>> break;
>> }
>> }
>>
>> - rc = gVBoxAPI.UISession.Open(data, &machineId, machine);
>> -
>> - if (NS_FAILED(rc)) {
>> + if (NS_FAILED(gVBoxAPI.UISession.Open(data, &machineId, machine)))
{
>> vboxIIDUnalloc(&machineId);
>> continue;
>> }
>>
>> - rc = gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
>> - if (NS_FAILED(rc))
>> + if (NS_FAILED(gVBoxAPI.UISession.GetMachine(data->vboxSession,
>> + &machine)))
>> goto cleanupLoop;
>>
>> gVBoxAPI.UArray.vboxArrayGet(&hddAttachments, machine,
>> @@ -633,13 +632,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
unsigned int flags)
>> if (!hddAttachment)
>> continue;
>>
>> - rc = gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment,
&hdd);
>> - if (NS_FAILED(rc) || !hdd)
>> + if (NS_FAILED(gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment,
>> + &hdd)) ||
!hdd)
>> continue;
>>
>> VBOX_IID_INITIALIZE(&iid);
>> - rc = gVBoxAPI.UIMedium.GetId(hdd, &iid);
>> - if (NS_FAILED(rc)) {
>> + if (NS_FAILED(gVBoxAPI.UIMedium.GetId(hdd, &iid))) {
>> VBOX_MEDIUM_RELEASE(hdd);
>> continue;
>> }
>> @@ -658,9 +656,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
unsigned int flags)
>> gVBoxAPI.UIMediumAttachment.GetPort(hddAttachment, &port);
>> gVBoxAPI.UIMediumAttachment.GetDevice(hddAttachment,
&device);
>>
>> - rc = gVBoxAPI.UIMachine.DetachDevice(machine, controller, port,
device);
>> - if (NS_SUCCEEDED(rc)) {
>> - rc = gVBoxAPI.UIMachine.SaveSettings(machine);
>> + if (NS_SUCCEEDED(gVBoxAPI.UIMachine.DetachDevice(machine,
controller, port, device))) {
>> + ignore_value(gVBoxAPI.UIMachine.SaveSettings(machine));
>
> While this does work (eg, the ignore_value), is that the intention?
>
> I asked the author :
>
> http://www.redhat.com/archives/libvir-list/2014-October/msg01050.html
>
> if the intention was to ignore the 'rc' value or not? The question
> being of course if the SaveSettings fails, should the rest of the code
> be executed? I don't know what to expect in vBox..
>
> As painful as it is with the daily failing Coverity tests - I was hoping
> we could get an answer there first.
>
Yes I've noticed that email and it would be probably nice to get answer
to this question and also fix it if it should fail. If we get an answer
before release I can send a v2 or just update the code and push if there
is nothing else wrong with this patch. The reason I've sent those
patches now is to get a review for the resolution of coverity complains.
Pavel
> John
>> VIR_DEBUG("saving machine settings");
>> deregister++;
>> VIR_DEBUG("deregistering hdd:%d", deregister);
>> @@ -683,9 +680,8 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
unsigned int flags)
>>
>> if (machineIdsSize == 0 || machineIdsSize == deregister) {
>> IProgress *progress = NULL;
>> - rc = gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress);
>> -
>> - if (NS_SUCCEEDED(rc) && progress) {
>> + if (NS_SUCCEEDED(gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk,
&progress)) &&
>> + progress) {
>> gVBoxAPI.UIProgress.WaitForCompletion(progress, -1);
>> VBOX_RELEASE(progress);
>> DEBUGIID("HardDisk deleted, UUID", &hddIID);
>>
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
11:45 a.m.
New subject: [libvirt] [PATCH 2/2] hotplug: fix char device detach
Hotplugging and hotunplugging char devices is only supported through
'-device' and the check for device capability should be independently.
Coverity also complains about 'tmpChr->info.alias' could be NULL and we
are dereferencing it but it somehow only in this case don't recognize
that the value is set by 'qemuAssignDeviceChrAlias' so it's clearly
false positive. Add comment to skip this error in this case.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d3bf392..520c0db 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3713,18 +3713,22 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
return ret;
}
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
- !tmpChr->info.alias) {
- if (qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0)
- return ret;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("qemu does not support -device"));
+ return ret;
}
+ if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1)
< 0)
+ return ret;
+
if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0)
return ret;
qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
qemuDomainObjEnterMonitor(driver, vm);
+ /* coverity[var_deref_model] */
if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) <
0) {
qemuDomainObjExitMonitor(driver, vm);
goto cleanup;
--
2.0.4
12:16 p.m.
New subject: [libvirt] [PATCH 2/2] hotplug: fix char device detach
On 10/31/2014 12:45 PM, Pavel Hrdina wrote:
Hotplugging and hotunplugging char devices is only supported through
'-device' and the check for device capability should be independently.
Coverity also complains about 'tmpChr->info.alias' could be NULL and we
are dereferencing it but it somehow only in this case don't recognize
that the value is set by 'qemuAssignDeviceChrAlias' so it's clearly
false positive. Add comment to skip this error in this case.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d3bf392..520c0db 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3713,18 +3713,22 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
return ret;
}
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
- !tmpChr->info.alias) {
- if (qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0)
- return ret;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("qemu does not support -device"));
+ return ret;
}
Doh! I should have looked at the Attach code which has this check first
and done the same thing. Instead I was following the model of other
patches in my series.
You can choose to leave it there or hoist it as first check, too
+ if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1)
< 0)
+ return ret;
+
Add 'sa_assert(tmpChr->info.alias);' see [1]
if (qemuBuildChrDeviceStr(&devstr, vm->def, chr,
priv->qemuCaps) < 0)
return ret;
qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
qemuDomainObjEnterMonitor(driver, vm);
+ /* coverity[var_deref_model] */
[1] This could be too generic...
By adding the sa_assert() above which only gets executed under Coverity
you avoid the need for this
ACK with those changes
John
if (devstr && qemuMonitorDelDevice(priv->mon,
tmpChr->info.alias) < 0) {
qemuDomainObjExitMonitor(driver, vm);
goto cleanup;
12:30 p.m.
New subject: [libvirt] [PATCH 2/2] hotplug: fix char device detach
On 10/31/2014 06:16 PM, John Ferlan wrote:
On 10/31/2014 12:45 PM, Pavel Hrdina wrote:
> Hotplugging and hotunplugging char devices is only supported through
> '-device' and the check for device capability should be independently.
>
> Coverity also complains about 'tmpChr->info.alias' could be NULL and we
> are dereferencing it but it somehow only in this case don't recognize
> that the value is set by 'qemuAssignDeviceChrAlias' so it's clearly
> false positive. Add comment to skip this error in this case.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index d3bf392..520c0db 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3713,18 +3713,22 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
> return ret;
> }
>
> - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
> - !tmpChr->info.alias) {
> - if (qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0)
> - return ret;
> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("qemu does not support -device"));
> + return ret;
> }
Doh! I should have looked at the Attach code which has this check first
and done the same thing. Instead I was following the model of other
patches in my series.
You can choose to leave it there or hoist it as first check, too
>
> + if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr,
-1) < 0)
> + return ret;
> +
Add 'sa_assert(tmpChr->info.alias);' see [1]
> if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) <
0)
> return ret;
>
> qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
>
> qemuDomainObjEnterMonitor(driver, vm);
> + /* coverity[var_deref_model] */
[1] This could be too generic...
By adding the sa_assert() above which only gets executed under Coverity
you avoid the need for this
ACK with those changes
Oh, that's better solution. I'll update the patch and push it tomorrow
if we don't get any answer about the first patch.
Thanks,
Pavel
John
> if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias)
< 0) {
> qemuDomainObjExitMonitor(driver, vm);
> goto cleanup;
>
3706
days inactive
3706
days old
7 comments
2 participants
participants (2)
-
John Ferlan -
Pavel Hrdina