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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list