[libvirt] [PATCHv3 2/2] interface: add udev based backend for virInterface

[Eric Blake]

On 09/17/2012 07:27 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()

I know there was some initial hesitation when you posted v2, but I
personally like the idea.  I'll wait another 24 hours for feedback, in
case anyone is worried that this is too close to the release and/or not
the right thing to be doing.  If we get other affirmative response (or
even silence), then I'll probably apply a fixed v4 of this patch, as
udev is indeed nicer than HAL for a fallback when netcf is not present.

Again, missing a change to po/POTFILES.in, based on 'make syntax-check'.

> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit 440a1dbe523e37f206252cb034c3a62f26867e42
> +Subproject commit 271dd74fdf54ec2a03e73a5173b0b5697f6088f1

You do NOT want a gnulib submodule update in this patch.

> diff --git a/configure.ac b/configure.ac
> index 1a44d21..e7757dd 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2803,11 +2803,15 @@ if test "$with_interface:$with_netcf" = "check:yes" ; then
>  fi
>  
>  if test "$with_interface:$with_netcf" = "check:no" ; then
> -  with_interface=no
> +  if test "$with_udev" = "yes" ; then
> +    with_interface=yes
> +  else
> +    with_interface=no
> +  fi
>  fi
>  
> -if test "$with_interface:$with_netcf" = "yes:no" ; then
> -  AC_MSG_ERROR([Requested the Interface driver without netcf support])
> +if test "$with_interface:$with_netcf:$with_udev" = "yes:no:no" ; then
> +  AC_MSG_ERROR([Requested the Interface driver without netcf or udev support])
>  fi

This might be simpler to merge into a case statement:

case $with_interface:$with_netcf:$with_udev in
  check:*yes*) with_interface=yes ;;
  check:no:no) with_interface=no ;;
  yes:no:no) AC_MSG_ERROR(...) ;;
esac

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

Another syntax-check failure:

trailing_blank
src/interface/interface_backend_udev.c:58:
src/interface/interface_backend_udev.c:193:
src/interface/interface_backend_udev.c:277:
src/interface/interface_backend_udev.c:320:}
src/interface/interface_backend_udev.c:377:}

> +
> +    /* We don't want to see the TUN devices that QEMU creates for other gets

s/gets/guests/

> +static virDrvOpenStatus
> +udevIfaceOpenInterface(virConnectPtr conn,
> +                       virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> +                       unsigned int flags ATTRIBUTE_UNUSED)
> +{

Fails 'make syntax-check':

src/interface/interface_backend_udev.c:85:
unsigned int flags ATTRIBUTE_UNUSED)
maint.mk: flags should be checked with virCheckFlags

You need to remove the attribute, and add virCheckFlags(0,
VIR_DRV_OPEN_ERROR), if you truly don't care about the flags.


> +static int
> +udevIfaceListInterfaces(virConnectPtr conn, char **const names, int names_len)
> +{

> +    /* For each item so we can count */
> +    udev_list_entry_foreach(dev_entry, devices) {
> +        struct udev_device *dev;
> +        const char *path;
> +        
> +        path = udev_list_entry_get_name(dev_entry);
> +        dev = udev_device_new_from_syspath(udev, path);
> +        names[count] = strdup(udev_device_get_sysname(dev));

Missing a check for allocation error and a subsequent
virReportOOMError().  If it fails midway through the loop, you also have
to clean up the partial results.

> +static int
> +udevIfaceListDefinedInterfaces(virConnectPtr conn,
> +                               char **const names,
> +                               int names_len)
> +{

> +
> +    /* For each item so we can count */
> +    udev_list_entry_foreach(dev_entry, devices) {
> +        struct udev_device *dev;
> +        const char *path;
> +        
> +        path = udev_list_entry_get_name(dev_entry);
> +        dev = udev_device_new_from_syspath(udev, path);
> +        names[count] = strdup(udev_device_get_sysname(dev));

Another strdup that must be checked.

> +static virInterfaceDriver udevIfaceDriver = {
> +    "Interface",
> +    .open = udevIfaceOpenInterface, /* 0.7.0 */
> +    .close = udevIfaceCloseInterface, /* 0.7.0 */
> +    .numOfInterfaces = udevIfaceNumOfInterfaces, /* 0.7.0 */
> +    .listInterfaces = udevIfaceListInterfaces, /* 0.7.0 */
> +    .numOfDefinedInterfaces = udevIfaceNumOfDefinedInterfaces, /* 0.7.0 */
> +    .listDefinedInterfaces = udevIfaceListDefinedInterfaces, /* 0.7.0 */
> +    .interfaceLookupByName = udevIfaceLookupByName, /* 0.7.0 */
> +    .interfaceLookupByMACString = udevIfaceLookupByMACString, /* 0.7.0 */

0.10.2 (if it goes into this release) or 0.10.3 for all of these comments.

> +++ b/tools/virsh.c
> @@ -2712,6 +2712,9 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED)
>  #if defined(WITH_NETCF)
>      vshPrint(ctl, " netcf");
>  #endif
> +#if !defined(WITH_NETCF) && defined(HAVE_UDEV)
> +    vshPrint(ctl, " udev");
> +#endif
>  #endif

Rather than nested #if, here, I'd go with:

# if defined(WITH_NETCF)
    vshPrint(ctl, " netcf");
# elif defined(HAVE_UDEV)
    vshPrint(ctl, " udev");
# endif

(indented to keep cppi happy, another one of those syntax checks).

-- 
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/20120919/3f0d5e28/attachment-0001.sig>