On 09/04/2012 12:10 PM, Osier Yang wrote:
> This is not that ideal as API for other objects, as it's still
> O(n).
That part I don't see as a big issue, since the number of interfaces
isn't generally large enough to have any significant impact :-)
> 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
> ......
The one big difference being that the netcf API allows you to get both
active and inactive interfaces in a single call.
>
> Perhaps we should further hack netcf to let it provide an API
s/hack/improve/ :-)
> to return the object, but it could be a later patch. And anyway,
> we will still befinit from the new API for the simplification,
s/benifit/benefit/
> and no race like the old APIs.
There's really no way to eliminate all races, because the interfaces
that netcf is reporting can simultaneously be modified by any other
process on the system that has root access - netcf is just reporting
back what's in the ifcfg files, and something like NetworkManager (or a
user with emacs) could modify/remove the files while netcf is building
its own list. At best you can just narrow the window, and the utility of
doing that is dubious because, in practice, modifications don't really
happen that often anyway.
>
> src/interface/netcf_driver.c: Implement listAllInterfaces
> ---
> src/interface/netcf_driver.c | 135 ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 135 insertions(+), 0 deletions(-)
>
> diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c
> index 935be66..1dae99b 100644
> --- a/src/interface/netcf_driver.c
> +++ b/src/interface/netcf_driver.c
> @@ -259,6 +259,140 @@ 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;
> + }
> +
> + if (count == 0)
> + return 0;
> +
> + if (VIR_ALLOC_N(names, count) < 0) {
> + virReportOOMError();
> + return -1;
> + }
If you want to check for a race here, you could allocate count+1 items
in the array, then do an ncf_list_interfaces telling it the maxcount is
"count+1" and check to see that you really only got count items back.
> +
> + 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 : "");
> + } else {
> + virReportError(VIR_ERR_NO_INTERFACE,
> + _("couldn't find interface named '%s'"), names[i]);
Since, as I said above, it's possible for another process to delete an
interface in between getting the list of all interfaces and querying
each individual interface, I think an error here should just result in
removing that interface from the list.
> + }
> + goto cleanup;
> + }
> +
> + 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 fitler flags
Okay. s/fitler/filter/g
> + * 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++;
> + }
You're leaking one ncf_if for each iteration through this loop. You need
to do
ncf_if_free(iface)
iface = NULL; /* make the final ncf_if_free() in cleanup: harmless */
after each iteration of this loop.
> + }
> +
> + 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 +776,7 @@ static virInterfaceDriver interfaceDriver = {
> .listInterfaces = interfaceListInterfaces, /* 0.7.0 */
> .numOfDefinedInterfaces = interfaceNumOfDefinedInterfaces, /* 0.7.0 */
> .listDefinedInterfaces = interfaceListDefinedInterfaces, /* 0.7.0 */
> + .listAllInterfaces = interfaceListAllInterfaces, /* 0.10.0 */
0.10.2
> .interfaceLookupByName = interfaceLookupByName, /* 0.7.0 */
> .interfaceLookupByMACString = interfaceLookupByMACString, /* 0.7.0 */
> .interfaceGetXMLDesc = interfaceGetXMLDesc, /* 0.7.0 */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list