[libvirt] [PATCH 3/5] list: Implement listAllInterfaces
Osier Yang
jyang at redhat.com
Wed Sep 12 03:05:50 UTC 2012
On 2012年09月11日 01:29, Laine Stump wrote:
> 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/ :-)
Okay.
>
>> 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/
Okay.
>
>
>> 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.
Understood.
>
>
>>
>> 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.
No want I think, :-)
Since I will adopt your below suggestion to igore the no found interface
by "ncf_lookup_by_name", that means we should either ignore the race, or
raise up error earlier here.
I think ignoring is better, any objections?
>
>> +
>> + 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.
Agreed, but no removing, as it's not inserted yet, just continue the
loop works. And instead of an error, a warning might be more proper.
>
>
>> + }
>> + 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
Okay, my fingers always make mistake on "Filter". :(
>
>
>> + * 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.
Good catch! Thanks.
>
>> + }
>> +
>> + 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
Okay.
>
>
>> .interfaceLookupByName = interfaceLookupByName, /* 0.7.0 */
>> .interfaceLookupByMACString = interfaceLookupByMACString, /* 0.7.0 */
>> .interfaceGetXMLDesc = interfaceGetXMLDesc, /* 0.7.0 */
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list