[libvirt] [PATCHv5] interface: add udev based backend for virInterface

[Eric Blake]

On 10/05/2012 12:43 PM, Doug Goldstein wrote:
> Add a read-only udev based backend for virInterface. Useful for distros
> that do not have netcf support yet. Multiple libvirt based utilities use
> a HAL based fallback when virInterface is not available which is less
> than ideal. This implements:
> * virConnectNumOfInterfaces()
> * virConnectListInterfaces()
> * virConnectNumOfDefinedInterfaces()
> * virConnectListDefinedInterfaces()
> * virConnectInterfaceLookupByName()
> * virConnectInterfaceLookupByMACString()
> ---

We really should also have virConnectListAllInterfaces() supported as
part of this patch.

> +++ b/src/Makefile.am
> @@ -558,11 +558,16 @@ INTERFACE_DRIVER_SOURCES =
>  if WITH_INTERFACE
>  INTERFACE_DRIVER_SOURCES +=					\
>  		interface/interface_driver.h
> -endif
>  
>  if WITH_NETCF
>  INTERFACE_DRIVER_SOURCES +=					\
>  		interface/interface_backend_netcf.c
> +else
> +if HAVE_UDEV
> +INTERFACE_DRIVER_SOURCES +=					\
> +		interface/interface_backend_udev.c

Hmm, this means the file isn't compiled on machines that have both netcf
and udev development support, which means it is prone to bit-rot.  I'd
feel a little bit more comfortable if we had a file interface_driver.c
that implemented the real driver.h callbacks, and then further forwarded
on to internal callbacks, so that both backend files can be compiled at
once (even if the code in the udev backend is not used).

[Or put another way, I don't want to have to configure --without-netcf
just to test your new code, as that doubles maintenance burden.]

However, as written, this part of your patch is okay, and we could save
breaking things into a two-layer callback structure to ensure full
compilation for a later patch, although I'd still like that patch to be
present before pushing this one (so we don't forget the issue).

> +++ b/src/interface/interface_backend_udev.c

> +enum udev_status { UDEV_IFACE_ACTIVE, UDEV_IFACE_INACTIVE, UDEV_IFACE_ALL };

Are these enum names going to possibly conflict with udev namespace?
You might want to name them VIR_UDEV_*, to ensure that we won't be
setting ourselves up for conflicts.  Also, I think the tag is an unusual
name, I'd rather see:

typedef enum {
   VIR_UDEV_...,
} virUdevStatus;

> +
> +static struct udev_enumerate *
> +udevIfaceGetDevices(struct udev *udev, enum udev_status status)

and then here, you could just use (struct udev *udev, virUdevStatus status)

> +{
> +    struct udev_enumerate *enumerate;
> +
> +    /* Check input params for sanity */
> +    if (!udev)
> +        return NULL;
> +
> +    /* Create a new enumeration to create a list */
> +    enumerate = udev_enumerate_new(udev);
> +
> +    if (!enumerate)
> +        return NULL;

You are returning NULL for two different reasons; do you need to use
virReportError() for better results back to the user?

> +
> +    /* Enumerate all network subsystem devices */
> +    udev_enumerate_add_match_subsystem(enumerate, "net");
> +
> +    /* Ignore devices that are part of a bridge */
> +    udev_enumerate_add_nomatch_sysattr(enumerate, "brport/state", NULL);
> +
> +    /* State of the device */
> +    switch (status) {
> +        case UDEV_IFACE_ACTIVE:
> +            udev_enumerate_add_match_sysattr(enumerate, "operstate", "up");
> +            break;
> +
> +        case UDEV_IFACE_INACTIVE:
> +            udev_enumerate_add_match_sysattr(enumerate, "operstate", "down");
> +            break;
> +
> +        case UDEV_IFACE_ALL:
> +            break;
> +    }
> +
> +    /* We don't want to see the TUN devices that QEMU creates for other gets

s/gets/guests/

> +static int
> +udevIfaceNumOfDefinedInterfaces(virConnectPtr conn)
> +{
> +    struct udev_iface_driver *driverState = conn->interfacePrivateData;
> +    struct udev *udev = udev_ref(driverState->udev);
> +    struct udev_enumerate *enumerate = NULL;
> +    struct udev_list_entry *devices;
> +    struct udev_list_entry *dev_entry;
> +    int count = 0;
> +
> +    enumerate = udevIfaceGetDevices(udev, UDEV_IFACE_INACTIVE);
> +
> +    if (!enumerate) {

NumOfInterfaces and NumOfDefinedInterfaces look very similar; to avoid
code duplication, I'd consider a static helper function that takes one
additional argument (UDEV_IFACE_ACTIVE or UDEV_IFACE_INACTIVE), since
that's all the difference between them.  Same thing for listing names.

Overall looks relatively straightforward.  The missing support for
ListAllInterfaces is worth a v6, though.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121005/262d71f7/attachment-0001.sig>