Re: [PATCH 1/2] vbox_storage: fix coverity issue with overwriting value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
>>> ---
>>>   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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> 

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]