Re: [libvirt-python PATCH 18/23] improve usage of cleanup paths

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

 



On Sat, Sep 26, 2015 at 10:14:37AM -0400, John Ferlan wrote:
> 
> 
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > This removes several code duplicates and also some unusual code structures.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> >  libvirt-override.c | 159 +++++++++++++++++++++++------------------------------
> >  1 file changed, 68 insertions(+), 91 deletions(-)
> > 
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 8afe7d7..205f0dd 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -439,13 +439,13 @@ libvirt_virDomainGetSchedulerType(PyObject *self ATTRIBUTE_UNUSED,
> >          return VIR_PY_NONE;
> >  
> >      /* convert to a Python tuple of long objects */
> > -    if ((info = PyTuple_New(2)) == NULL) {
> > -        VIR_FREE(c_retval);
> > -        return NULL;
> > -    }
> > +    if ((info = PyTuple_New(2)) == NULL)
> > +        goto cleanup;
> >  
> >      PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(c_retval));
> >      PyTuple_SetItem(info, 1, libvirt_intWrap((long)nparams));
> > +
> > + cleanup:
> >      VIR_FREE(c_retval);
> >      return info;
> >  }
> > @@ -2401,7 +2401,6 @@ libvirt_virDomainSnapshotListNames(PyObject *self ATTRIBUTE_UNUSED,
> >              Py_CLEAR(py_retval);
> >              goto cleanup;
> >          }
> > -        VIR_FREE(names[i]);
> >      }
> >  
> >   cleanup:
> > @@ -3245,8 +3244,8 @@ libvirt_virNodeGetCellsFreeMemory(PyObject *self ATTRIBUTE_UNUSED,
> >      LIBVIRT_END_ALLOW_THREADS;
> >  
> >      if (c_retval < 0) {
> > -        VIR_FREE(freeMems);
> > -        return VIR_PY_NONE;
> > +        py_retval = VIR_PY_NONE;
> > +        goto cleanup;
> >      }
> >  
> >      if ((py_retval = PyList_New(c_retval)) == NULL)
> > @@ -3425,24 +3424,19 @@ libvirt_virConnectListStoragePools(PyObject *self ATTRIBUTE_UNUSED,
> >              return VIR_PY_NONE;
> >          }
> >      }
> > -    py_retval = PyList_New(c_retval);
> > -    if (py_retval == NULL) {
> > -        if (names) {
> > -            for (i = 0; i < c_retval; i++)
> > -                VIR_FREE(names[i]);
> > -            VIR_FREE(names);
> > -        }
> > -        return NULL;
> > -    }
> > +
> > +    if ((py_retval = PyList_New(c_retval)) == NULL)
> > +        goto cleanup;
> >  
> >      if (names) {
> > -        for (i = 0; i < c_retval; i++) {
> > +        for (i = 0; i < c_retval; i++)
> >              PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
> > -            VIR_FREE(names[i]);
> > -        }
> > -        VIR_FREE(names);
> >      }
> >  
> > + cleanup:
> > +    for (i = 0; i < c_retval; i++)
> > +        VIR_FREE(names[i]);
> > +    VIR_FREE(names);
> >      return py_retval;
> >  }
> >  
> > @@ -3480,24 +3474,20 @@ libvirt_virConnectListDefinedStoragePools(PyObject *self ATTRIBUTE_UNUSED,
> >              return VIR_PY_NONE;
> >          }
> >      }
> > -    py_retval = PyList_New(c_retval);
> > -    if (py_retval == NULL) {
> > -        if (names) {
> > -            for (i = 0; i < c_retval; i++)
> > -                VIR_FREE(names[i]);
> > -            VIR_FREE(names);
> > -        }
> > -        return NULL;
> > -    }
> > +
> > +    if ((py_retval = PyList_New(c_retval)) == NULL)
> > +        goto cleanup;
> >  
> >      if (names) {
> > -        for (i = 0; i < c_retval; i++) {
> > +        for (i = 0; i < c_retval; i++)
> >              PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
> > -            VIR_FREE(names[i]);
> > -        }
> > -        VIR_FREE(names);
> >      }
> >  
> > + cleanup:
> > +    for (i = 0; i < c_retval; i++)
> > +        VIR_FREE(names[i]);
> > +    VIR_FREE(names);
> > +
> >      return py_retval;
> >  }
> >  
> > @@ -3579,28 +3569,24 @@ libvirt_virStoragePoolListVolumes(PyObject *self ATTRIBUTE_UNUSED,
> >          c_retval = virStoragePoolListVolumes(pool, names, c_retval);
> >          LIBVIRT_END_ALLOW_THREADS;
> >          if (c_retval < 0) {
> > -            VIR_FREE(names);
> > -            return VIR_PY_NONE;
> > +            py_retval = VIR_PY_NONE;
> > +            goto cleanup;
> 
> We're going to cleanup with c_retval == -1, right... and then doing the
> for loop to free names - could be a long time to complete.
> 
> I do note that this differs from other prior uses in this patch - that
> is other functions will keep the VIR_FREE(names); return VIR_PY_NONE; if
> c_retval < 0
> 
> At this point it may be worthwhile for you to check *all* such goto's
> for c_retval && loops to free memory...

Oh, I didn't realize this, because some functions have 'int i;' and others have
'size_t i'.  I've verified the 'i' declaration only in function with int and
conclude it's ok to use it this way.  This needs to be fixed, I'll send a v2.

Pavel

> 
> >          }
> >      }
> > -    py_retval = PyList_New(c_retval);
> > -    if (py_retval == NULL) {
> > -        if (names) {
> > -            for (i = 0; i < c_retval; i++)
> > -                VIR_FREE(names[i]);
> > -            VIR_FREE(names);
> > -        }
> > -        return NULL;
> > -    }
> > +
> > +    if ((py_retval = PyList_New(c_retval)) == NULL)
> > +        goto cleanup;
> >  
> >      if (names) {
> > -        for (i = 0; i < c_retval; i++) {
> > +        for (i = 0; i < c_retval; i++)
> >              PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
> > -            VIR_FREE(names[i]);
> > -        }
> > -        VIR_FREE(names);
> >      }
> >  
> > + cleanup:
> > +    for (i = 0; i < c_retval; i++)
> > +        VIR_FREE(names[i]);
> > +    VIR_FREE(names);
> > +
> >      return py_retval;
> >  }
> >  
> > @@ -3850,9 +3836,10 @@ libvirt_virNodeListDevices(PyObject *self ATTRIBUTE_UNUSED,
> >          LIBVIRT_BEGIN_ALLOW_THREADS;
> >          c_retval = virNodeListDevices(conn, cap, names, c_retval, flags);
> >          LIBVIRT_END_ALLOW_THREADS;
> > +
> >          if (c_retval < 0) {
> > -            VIR_FREE(names);
> > -            return VIR_PY_NONE;
> > +            py_retval = VIR_PY_NONE;
> > +            goto cleanup;
> 
> oh - look at that... here's a similar comment regarding c_retval as above.
> 
> >          }
> >      }
> >  
> > @@ -3947,8 +3934,8 @@ libvirt_virNodeDeviceListCaps(PyObject *self ATTRIBUTE_UNUSED,
> >          c_retval = virNodeDeviceListCaps(dev, names, c_retval);
> >          LIBVIRT_END_ALLOW_THREADS;
> >          if (c_retval < 0) {
> > -            VIR_FREE(names);
> > -            return VIR_PY_NONE;
> > +            py_retval = VIR_PY_NONE;
> > +            goto cleanup;
> 
> again
> 
> >          }
> >      }
> >  
> > @@ -4072,8 +4059,8 @@ libvirt_virConnectListSecrets(PyObject *self ATTRIBUTE_UNUSED,
> >          c_retval = virConnectListSecrets(conn, uuids, c_retval);
> >          LIBVIRT_END_ALLOW_THREADS;
> >          if (c_retval < 0) {
> > -            VIR_FREE(uuids);
> > -            return VIR_PY_NONE;
> > +            py_retval = VIR_PY_NONE;
> > +            goto cleanup;
> 
> again
> 
> >          }
> >      }
> >  
> > @@ -4404,24 +4391,19 @@ libvirt_virConnectListInterfaces(PyObject *self ATTRIBUTE_UNUSED,
> >              return VIR_PY_NONE;
> >          }
> >      }
> 
> This one doesn't...
> 
> > -    py_retval = PyList_New(c_retval);
> > -    if (py_retval == NULL) {
> > -        if (names) {
> > -            for (i = 0; i < c_retval; i++)
> > -                VIR_FREE(names[i]);
> > -            VIR_FREE(names);
> > -        }
> > -        return NULL;
> > -    }
> > +
> > +    if ((py_retval = PyList_New(c_retval)) == NULL)
> > +        goto cleanup;
> >  
> >      if (names) {
> > -        for (i = 0; i < c_retval; i++) {
> > +        for (i = 0; i < c_retval; i++)
> >              PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
> > -            VIR_FREE(names[i]);
> > -        }
> > -        VIR_FREE(names);
> >      }
> >  
> > + cleanup:
> > +    for (i = 0; i < c_retval; i++)
> > +        VIR_FREE(names[i]);
> > +    VIR_FREE(names);
> >      return py_retval;
> >  }
> >  
> > @@ -4461,24 +4443,19 @@ libvirt_virConnectListDefinedInterfaces(PyObject *self ATTRIBUTE_UNUSED,
> >              return VIR_PY_NONE;
> >          }
> >      }
> > -    py_retval = PyList_New(c_retval);
> > -    if (py_retval == NULL) {
> > -        if (names) {
> > -            for (i = 0; i < c_retval; i++)
> > -                VIR_FREE(names[i]);
> > -            VIR_FREE(names);
> > -        }
> > -        return NULL;
> > -    }
> > +
> > +    if ((py_retval = PyList_New(c_retval)) == NULL)
> > +        goto cleanup;
> >  
> >      if (names) {
> > -        for (i = 0; i < c_retval; i++) {
> > +        for (i = 0; i < c_retval; i++)
> >              PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
> > -            VIR_FREE(names[i]);
> > -        }
> > -        VIR_FREE(names);
> >      }
> >  
> > + cleanup:
> > +    for (i = 0; i < c_retval; i++)
> > +        VIR_FREE(names[i]);
> > +    VIR_FREE(names);
> >      return py_retval;
> >  }
> >  
> > @@ -8459,17 +8436,19 @@ libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED,
> >  
> >      /* convert to a Python list */
> >      if ((py_retval = PyList_New(c_retval)) == NULL)
> > -        goto cleanup;
> > +        goto error;
> >  
> >      for (i = 0; i < c_retval; i++) {
> >          virDomainFSInfoPtr fs = fsinfo[i];
> >          PyObject *info, *alias;
> >  
> >          if (fs == NULL)
> > -            goto cleanup;
> > +            goto error;
> > +
> >          info = PyTuple_New(4);
> >          if (info == NULL)
> > -            goto cleanup;
> > +            goto error;
> > +
> >          PyList_SetItem(py_retval, i, info);
> >  
> >          PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(fs->mountpoint));
> > @@ -8478,27 +8457,25 @@ libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED,
> >  
> >          alias = PyList_New(0);
> >          if (alias == NULL)
> > -            goto cleanup;
> > +            goto error;
> >  
> >          PyTuple_SetItem(info, 3, alias);
> >  
> >          for (j = 0; j < fs->ndevAlias; j++)
> >              if (PyList_Append(alias,
> >                                libvirt_constcharPtrWrap(fs->devAlias[j])) < 0)
> > -                goto cleanup;
> > +                goto error;
> >      }
> >  
> 
> Perhaps this one was pre-existing, but if c_retval < 0, this isn't going
> to go well... or it won't be quick!
> 
> 
> John
> > + cleanup:
> >      for (i = 0; i < c_retval; i++)
> >          virDomainFSInfoFree(fsinfo[i]);
> >      VIR_FREE(fsinfo);
> >      return py_retval;
> >  
> > - cleanup:
> > -    for (i = 0; i < c_retval; i++)
> > -        virDomainFSInfoFree(fsinfo[i]);
> > -    VIR_FREE(fsinfo);
> > -    Py_XDECREF(py_retval);
> > -    return NULL;
> > + error:
> > +    Py_CLEAR(py_retval);
> > +    goto cleanup;
> >  }
> >  
> >  #endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */
> > 

--
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]