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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org