[libvirt] [PATCH 1/2] vbox_storage: fix coverity issue with overwriting value
John Ferlan
jferlan at redhat.com
Fri Oct 31 17:46:46 UTC 2014
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 at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>
More information about the libvir-list
mailing list