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

[Pavel Hrdina <phrdina@xxxxxxxxxx>]

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(-)
> >
> > 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