5:29 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org