Re: [libvirt] [PATCH 4/6 v5] list: Implement listAllInterfaces

On 09/11/2012 11:37 PM, Osier Yang wrote: > This is not that ideal as API for other objects, as it's still > O(n). Because interface driver uses netcf APIs to manage the > stuffs, instead of by itself. And netcf APIs don't return a object. > It provides APIs like old libvirt APIs: > > ncf_number_of_interfaces > ncf_list_interfaces > ncf_lookup_by_name > ...... > > Perhaps we should further improve netcf to let it provide an API > to return the object, but it could be a later patch. And anyway, > we will still benefit from the new API for the simplification, > and no race like the old APIs. > > src/interface/netcf_driver.c: Implement listAllInterfaces > > v4 - v5: > * Ignore the NETCF_NOERROR, in case of the iface could be deleted > by other management apps as a race. > > * Fix the memory leak caught by Laine. > > * The version number fix. > > --- > src/interface/netcf_driver.c | 143 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 143 insertions(+), 0 deletions(-) > > diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c > index 935be66..26d25d7 100644 > --- a/src/interface/netcf_driver.c > +++ b/src/interface/netcf_driver.c > @@ -30,6 +30,7 @@ > #include "netcf_driver.h" > #include "interface_conf.h" > #include "memory.h" > +#include "logging.h" > > #define VIR_FROM_THIS VIR_FROM_INTERFACE > > @@ -259,6 +260,147 @@ static int interfaceListDefinedInterfaces(virConnectPtr conn, char **const names > > } > > +static int > +interfaceListAllInterfaces(virConnectPtr conn, > + virInterfacePtr **ifaces, > + unsigned int flags) > +{ > + struct interface_driver *driver = conn->interfacePrivateData; > + int count; > + int i; > + struct netcf_if *iface = NULL; > + virInterfacePtr *tmp_iface_objs = NULL; > + virInterfacePtr iface_obj = NULL; > + unsigned int status; > + int niface_objs = 0; > + int ret = -1; > + char **names; > + > + virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE | > + VIR_CONNECT_LIST_INTERFACES_INACTIVE, -1); > + > + interfaceDriverLock(driver); > + > + /* List all interfaces, in case of we might support new filter flags > + * except active|inactive in future. > + */ > + count = ncf_num_of_interfaces(driver->netcf, NETCF_IFACE_ACTIVE | > + NETCF_IFACE_INACTIVE); > + if (count< 0) { > + const char *errmsg, *details; > + int errcode = ncf_error(driver->netcf,&errmsg,&details); > + virReportError(netcf_to_vir_err(errcode), > + _("failed to get number of host interfaces: %s%s%s"), > + errmsg, details ? " - " : "", > + details ? details : ""); > + return -1; Ooh! I just noticed that you have several return paths here that don't call interfaceDriverUnlock()!! If you initialize names = NULL, then qualify the loop to free names with "if (names) { ... }", you should be able to just to "ret = X; goto cleanup;" instead of return.