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.
> + }
> +
> + if (count == 0)
> + return 0;
> +
> + if (VIR_ALLOC_N(names, count) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if ((count = ncf_list_interfaces(driver->netcf, count, names,
> + NETCF_IFACE_ACTIVE |
> + NETCF_IFACE_INACTIVE)) < 0) {
> + const char *errmsg, *details;
> + int errcode = ncf_error(driver->netcf, &errmsg, &details);
> + virReportError(netcf_to_vir_err(errcode),
> + _("failed to list host interfaces: %s%s%s"),
> + errmsg, details ? " - " : "",
> + details ? details : "");
> + goto cleanup;
> + }
> +
> + if (ifaces) {
> + if (VIR_ALLOC_N(tmp_iface_objs, count + 1) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + }
> +
> + for (i = 0; i < count; i++) {
> + iface = ncf_lookup_by_name(driver->netcf, names[i]);
> + if (!iface) {
> + const char *errmsg, *details;
> + int errcode = ncf_error(driver->netcf, &errmsg, &details);
> + if (errcode != NETCF_NOERROR) {
> + virReportError(netcf_to_vir_err(errcode),
> + _("couldn't find interface named '%s': %s%s%s"),
> + names[i], errmsg,
> + details ? " - " : "", details ? details : "");
> + goto cleanup;
> + } else {
> + /* Ignore the NETCF_NOERROR, as the interface is very likely
> + * deleted by other management apps (e.g. virt-manager).
> + */
> + VIR_WARN("couldn't find interface named '%s', might be "
> + "deleted by other process", names[i]);
Yeah, I guess this is uncommon enough (as in "I bet this will never
happen, unless I actually put money on the bet" :-) that it's okay to
print a warning.
> + continue;
> + }
> + }
> +
> + if (ncf_if_status(iface, &status) < 0) {
> + const char *errmsg, *details;
> + int errcode = ncf_error(driver->netcf, &errmsg, &details);
> + virReportError(netcf_to_vir_err(errcode),
> + _("failed to get status of interface %s: %s%s%s"),
> + names[i], errmsg, details ? " - " : "",
> + details ? details : "");
> + goto cleanup;
> + }
> +
> + /* XXX: Filter the result, need to be splitted once new filter flags
> + * except active|inactive are supported.
> + */
> + if (((status & NETCF_IFACE_ACTIVE) &&
> + (flags & VIR_CONNECT_LIST_INTERFACES_ACTIVE)) ||
> + ((status & NETCF_IFACE_INACTIVE) &&
> + (flags & VIR_CONNECT_LIST_INTERFACES_INACTIVE))) {
> + if (ifaces) {
> + iface_obj = virGetInterface(conn, ncf_if_name(iface),
> + ncf_if_mac_string(iface));
> + tmp_iface_objs[niface_objs] = iface_obj;
> + }
> + niface_objs++;
> + }
> +
> + ncf_if_free(iface);
> + iface = NULL;
> + }
> +
> + if (tmp_iface_objs) {
> + /* trim the array to the final size */
> + ignore_value(VIR_REALLOC_N(tmp_iface_objs, niface_objs + 1));
> + *ifaces = tmp_iface_objs;
> + tmp_iface_objs = NULL;
> + }
> +
> + ret = niface_objs;
> +
> +cleanup:
> + ncf_if_free(iface);
> +
> + for (i = 0; i < count; i++)
> + VIR_FREE(names[i]);
> + VIR_FREE(names);
> +
> + if (tmp_iface_objs) {
> + for (i = 0; i < niface_objs; i++) {
> + if (tmp_iface_objs[i])
> + virInterfaceFree(tmp_iface_objs[i]);
> + }
> + }
> +
> + interfaceDriverUnlock(driver);
> + return ret;
> +}
> +
> +
> static virInterfacePtr interfaceLookupByName(virConnectPtr conn,
> const char *name)
> {
> @@ -642,6 +784,7 @@ static virInterfaceDriver interfaceDriver = {
> .listInterfaces = interfaceListInterfaces, /* 0.7.0 */
> .numOfDefinedInterfaces = interfaceNumOfDefinedInterfaces, /* 0.7.0 */
> .listDefinedInterfaces = interfaceListDefinedInterfaces, /* 0.7.0 */
> + .listAllInterfaces = interfaceListAllInterfaces, /* 0.10.2 */
> .interfaceLookupByName = interfaceLookupByName, /* 0.7.0 */
> .interfaceLookupByMACString = interfaceLookupByMACString, /* 0.7.0 */
> .interfaceGetXMLDesc = interfaceGetXMLDesc, /* 0.7.0 */
ACK with the interfaceDriverUnlock() problem fixed.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list