[libvirt] [PATCHv3 1/2] build: define WITH_INTERFACE for the driver
Based exclusively on work by Eric Blake in a patch posted with the same subject. However some modifications related to comments and my plans to add another backend. Added WITH_INTERFACE as the only automake variable deciding whether to build the driver and using WITH_NETCF to identify that we're wanting to use the netcf library as the backend. * configure.ac: Added with_interface * src/interface/netcf_driver.c: Renamed.. * src/interface/interface_backend_netcf.c: ..to this to match storage. * src/interface/netcf_driver.h: Renamed.. * src/interface/interface_driver.h: ..to this. * daemon/Makefile.am: Respect WITH_INTERFACE and WITH_NETCF. * libvirt.spec.in: Add RPM support for --with-interface --- Change from v2: * rebase against master * v2 conditionally ACK'd by Laine if Eric ACK'd it Change from v1: * rebased against master * Fixed copyright header * Added libvirt.spec.in modification * Changed virsh -V output configure.ac | 33 ++++++++++++++++++- daemon/Makefile.am | 2 +- daemon/libvirtd.c | 8 ++-- libvirt.spec.in | 13 ++++--- src/Makefile.am | 24 +++++++++++--- .../{netcf_driver.c => interface_backend_netcf.c} | 2 +- .../{netcf_driver.h => interface_driver.h} | 0 tests/virdrivermoduletest.c | 2 +- tools/virsh.c | 5 ++- 9 files changed, 67 insertions(+), 22 deletions(-) rename src/interface/{netcf_driver.c => interface_backend_netcf.c} (99%) rename src/interface/{netcf_driver.h => interface_driver.h} (100%) diff --git a/configure.ac b/configure.ac index 2090e5f..1a44d21 100644 --- a/configure.ac +++ b/configure.ac @@ -1948,7 +1948,6 @@ AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS]) - AC_ARG_WITH([secrets], AC_HELP_STRING([--with-secrets], [with local secrets management driver @<:@default=yes@:>@]),[],[with_secrets=yes]) @@ -2787,6 +2786,36 @@ if test "$with_nwfilter" = "yes" ; then fi AM_CONDITIONAL([WITH_NWFILTER], [test "$with_nwfilter" = "yes"]) +dnl check if the interface driver should be compiled +AC_ARG_WITH([interface], + AC_HELP_STRING([--with-interface], + [with host interface driver @<:@default=check@:>@]), [], + [with_interface=check]) + +dnl Don't compile the interface driver without libvirtd +if test "$with_libvirtd" = "no" ; then + with_interface=no +fi + +dnl The interface driver depends on the netcf library +if test "$with_interface:$with_netcf" = "check:yes" ; then + with_interface=yes +fi + +if test "$with_interface:$with_netcf" = "check:no" ; then + with_interface=no +fi + +if test "$with_interface:$with_netcf" = "yes:no" ; then + AC_MSG_ERROR([Requested the Interface driver without netcf support]) +fi + +if test "$with_interface" = "yes" ; then + AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1], + [whether the interface driver is enabled]) +fi +AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"]) + dnl libblkid is used by several storage drivers; therefore we probe dnl for it unconditionally. AC_ARG_WITH([libblkid], @@ -3016,7 +3045,7 @@ AC_MSG_NOTICE([ Test: $with_test]) AC_MSG_NOTICE([ Remote: $with_remote]) AC_MSG_NOTICE([ Network: $with_network]) AC_MSG_NOTICE([ Libvirtd: $with_libvirtd]) -AC_MSG_NOTICE([ netcf: $with_netcf]) +AC_MSG_NOTICE([Interface: $with_interface]) AC_MSG_NOTICE([ macvtap: $with_macvtap]) AC_MSG_NOTICE([ virtport: $with_virtualport]) AC_MSG_NOTICE([]) diff --git a/daemon/Makefile.am b/daemon/Makefile.am index b45349c..3405c67 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -151,7 +151,7 @@ if WITH_NETWORK libvirtd_LDADD += ../src/libvirt_driver_network.la endif -if WITH_NETCF +if WITH_INTERFACE libvirtd_LDADD += ../src/libvirt_driver_interface.la endif diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 6973df6..1156bd6 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -78,8 +78,8 @@ # ifdef WITH_NETWORK # include "network/bridge_driver.h" # endif -# ifdef WITH_NETCF -# include "interface/netcf_driver.h" +# ifdef WITH_INTERFACE +# include "interface/interface_driver.h" # endif # ifdef WITH_STORAGE # include "storage/storage_driver.h" @@ -382,7 +382,7 @@ static void daemonInitialize(void) # ifdef WITH_NWFILTER virDriverLoadModule("nwfilter"); # endif -# ifdef WITH_NETCF +# ifdef WITH_INTERFACE virDriverLoadModule("interface"); # endif # ifdef WITH_XEN @@ -404,7 +404,7 @@ static void daemonInitialize(void) # ifdef WITH_NETWORK networkRegister(); # endif -# ifdef WITH_NETCF +# ifdef WITH_INTERFACE interfaceRegister(); # endif # ifdef WITH_STORAGE diff --git a/libvirt.spec.in b/libvirt.spec.in index 8c4c08d..853cef7 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -92,6 +92,7 @@ # A few optional bits off by default, we enable later %define with_polkit 0%{!?_without_polkit:0} %define with_capng 0%{!?_without_capng:0} +%define with_interface 0%{!?_without_interface:0} %define with_netcf 0%{!?_without_netcf:0} %define with_udev 0%{!?_without_udev:0} %define with_hal 0%{!?_without_hal:0} @@ -200,6 +201,12 @@ %define with_netcf 0%{!?_without_netcf:%{server_drivers}} %endif +# interface is the driver that wraps netcf or udev interface management +# backends in Fedora 18 / RHEL-7 or newer +%if 0%{?fedora} >= 18 || 0%{?rhel} >= 7 +%define with_interface 0%{!?_without_interface:%{server_drivers}} +%endif + # udev is used to manage host devices in Fedora 12 / RHEL-6 or newer %if 0%{?fedora} >= 12 || 0%{?rhel} >= 6 %define with_udev 0%{!?_without_udev:%{server_drivers}} @@ -281,12 +288,6 @@ %define with_nodedev 0 %endif -%if %{with_netcf} -%define with_interface 1 -%else -%define with_interface 0 -%endif - %if %{with_storage_fs} || %{with_storage_mpath} || %{with_storage_iscsi} || %{with_storage_lvm} || %{with_storage_disk} %define with_storage 1 %else diff --git a/src/Makefile.am b/src/Makefile.am index 9f27fcf..4ae741b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -552,8 +552,17 @@ PARALLELS_DRIVER_SOURCES = \ NETWORK_DRIVER_SOURCES = \ network/bridge_driver.h network/bridge_driver.c -INTERFACE_DRIVER_SOURCES = \ - interface/netcf_driver.h interface/netcf_driver.c +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 +endif SECRET_DRIVER_SOURCES = \ secret/secret_driver.h secret/secret_driver.c @@ -1021,7 +1030,7 @@ endif EXTRA_DIST += network/default.xml -if WITH_NETCF +if WITH_INTERFACE if WITH_DRIVER_MODULES mod_LTLIBRARIES += libvirt_driver_interface.la else @@ -1029,10 +1038,13 @@ noinst_LTLIBRARIES += libvirt_driver_interface.la # Stateful, so linked to daemon instead #libvirt_la_BUILT_LIBADD += libvirt_driver_interface.la endif -libvirt_driver_interface_la_CFLAGS = $(NETCF_CFLAGS) \ - -I$(top_srcdir)/src/conf $(AM_CFLAGS) +libvirt_driver_interface_la_CFLAGS = -I$(top_srcdir)/src/conf $(AM_CFLAGS) libvirt_driver_interface_la_LDFLAGS = $(AM_LDFLAGS) -libvirt_driver_interface_la_LIBADD = $(NETCF_LIBS) +libvirt_driver_interface_la_LIBADD = +if WITH_NETCF +libvirt_driver_interface_la_CFLAGS += $(NETCF_CFLAGS) +libvirt_driver_interface_la_LIBADD += $(NETCF_LIBS) +endif if WITH_DRIVER_MODULES libvirt_driver_interface_la_LIBADD += ../gnulib/lib/libgnu.la libvirt_driver_interface_la_LDFLAGS += -module -avoid-version diff --git a/src/interface/netcf_driver.c b/src/interface/interface_backend_netcf.c similarity index 99% rename from src/interface/netcf_driver.c rename to src/interface/interface_backend_netcf.c index 6e429db..3b4ee11 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/interface_backend_netcf.c @@ -27,7 +27,7 @@ #include "virterror_internal.h" #include "datatypes.h" -#include "netcf_driver.h" +#include "interface_driver.h" #include "interface_conf.h" #include "memory.h" #include "logging.h" diff --git a/src/interface/netcf_driver.h b/src/interface/interface_driver.h similarity index 100% rename from src/interface/netcf_driver.h rename to src/interface/interface_driver.h diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c index 4d6e91e..8762de4 100644 --- a/tests/virdrivermoduletest.c +++ b/tests/virdrivermoduletest.c @@ -79,7 +79,7 @@ mymain(void) #ifdef WITH_NWFILTER TEST("nwfilter", NULL); #endif -#ifdef WITH_NETCF +#ifdef WITH_INTERFACE TEST("interface", NULL); #endif #ifdef WITH_QEMU diff --git a/tools/virsh.c b/tools/virsh.c index d0b302a..6a7b89d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2707,8 +2707,11 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #ifdef WITH_BRIDGE vshPrint(ctl, " Bridging"); #endif -#ifdef WITH_NETCF +#if defined(WITH_INTERFACE) vshPrint(ctl, " Interface"); +#if defined(WITH_NETCF) + vshPrint(ctl, " netcf"); +#endif #endif #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter"); -- 1.7.8.6
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() --- Change from v2: * rebase from master Change from v1: * rebase from master .gnulib | 2 +- configure.ac | 10 +- src/Makefile.am | 12 +- src/interface/interface_backend_udev.c | 395 ++++++++++++++++++++++++++++++++ tools/virsh.c | 3 + 5 files changed, 417 insertions(+), 5 deletions(-) create mode 100644 src/interface/interface_backend_udev.c diff --git a/.gnulib b/.gnulib index 440a1db..271dd74 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 440a1dbe523e37f206252cb034c3a62f26867e42 +Subproject commit 271dd74fdf54ec2a03e73a5173b0b5697f6088f1 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 if test "$with_interface" = "yes" ; then diff --git a/src/Makefile.am b/src/Makefile.am index 4ae741b..53f1fbf 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -557,11 +557,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 +endif +endif endif SECRET_DRIVER_SOURCES = \ @@ -1044,6 +1049,11 @@ libvirt_driver_interface_la_LIBADD = if WITH_NETCF libvirt_driver_interface_la_CFLAGS += $(NETCF_CFLAGS) libvirt_driver_interface_la_LIBADD += $(NETCF_LIBS) +else +if HAVE_UDEV +libvirt_driver_interface_la_CFLAGS += $(UDEV_CFLAGS) +libvirt_driver_interface_la_LIBADD += $(UDEV_LIBS) +endif endif if WITH_DRIVER_MODULES libvirt_driver_interface_la_LIBADD += ../gnulib/lib/libgnu.la diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c new file mode 100644 index 0000000..30b71fd --- /dev/null +++ b/src/interface/interface_backend_udev.c @@ -0,0 +1,395 @@ +/* + * interface_backend_udev.c: udev backend for virInterface + * + * Copyright (C) 2012 Doug Goldstein <cardoe@cardoe.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + */ +#include <config.h> + +#include <libudev.h> + +#include "virterror_internal.h" +#include "datatypes.h" +#include "interface_driver.h" +#include "interface_conf.h" +#include "memory.h" + +#define VIR_FROM_THIS VIR_FROM_INTERFACE + +struct udev_iface_driver { + struct udev *udev; +}; + +enum udev_status { UDEV_IFACE_ACTIVE, UDEV_IFACE_INACTIVE, UDEV_IFACE_ALL }; + +static struct udev_enumerate * +udevIfaceGetDevices(struct udev *udev, enum udev_status 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; + + /* 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 + * running on this machine. By saying nomatch NULL, we just are getting + * devices without the tun_flags sysattr. + */ + udev_enumerate_add_nomatch_sysattr(enumerate, "tun_flags", NULL); + + return enumerate; +} + +static virDrvOpenStatus +udevIfaceOpenInterface(virConnectPtr conn, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct udev_iface_driver *driverState = NULL; + + if (VIR_ALLOC(driverState) < 0) { + virReportOOMError(); + goto err; + } + + driverState->udev = udev_new(); + if (!driverState->udev) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create udev context")); + goto err; + } + + conn->interfacePrivateData = driverState; + + return VIR_DRV_OPEN_SUCCESS; + +err: + VIR_FREE(driverState); + + return VIR_DRV_OPEN_ERROR; +} + +static int +udevIfaceCloseInterface(virConnectPtr conn) +{ + struct udev_iface_driver *driverState; + + if (conn->interfacePrivateData != NULL) { + driverState = conn->interfacePrivateData; + + udev_unref(driverState->udev); + + VIR_FREE(driverState); + } + + conn->interfacePrivateData = NULL; + return 0; +} + +static int +udevIfaceNumOfInterfaces(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_ACTIVE); + + if (!enumerate) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to get number of active interfaces on host")); + return -1; + } + + /* Do the scan to load up the enumeration */ + udev_enumerate_scan_devices(enumerate); + + /* Get a list we can walk */ + devices = udev_enumerate_get_list_entry(enumerate); + + /* For each item so we can count */ + udev_list_entry_foreach(dev_entry, devices) { + count++; + } + + udev_enumerate_unref(enumerate); + + udev_unref(udev); + + return count; +} + +static int +udevIfaceListInterfaces(virConnectPtr conn, char **const names, int names_len) +{ + 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_ACTIVE); + + if (!enumerate) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to get list of active interfaces on host")); + return -1; + } + + /* Do the scan to load up the enumeration */ + udev_enumerate_scan_devices(enumerate); + + /* Get a list we can walk */ + devices = udev_enumerate_get_list_entry(enumerate); + + /* 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)); + udev_device_unref(dev); + + count++; + if (count > names_len) + break; + } + + udev_enumerate_unref(enumerate); + + udev_unref(udev); + + return count; +} + +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) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to get number of defined interfaces on host")); + return -1; + } + + /* Do the scan to load up the enumeration */ + udev_enumerate_scan_devices(enumerate); + + /* Get a list we can walk */ + devices = udev_enumerate_get_list_entry(enumerate); + + /* For each item so we can count */ + udev_list_entry_foreach(dev_entry, devices) { + count++; + } + + udev_enumerate_unref(enumerate); + + udev_unref(udev); + + return count; +} + +static int +udevIfaceListDefinedInterfaces(virConnectPtr conn, + char **const names, + int names_len) +{ + 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) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to get list of defined interfaces on host")); + return -1; + } + + /* Do the scan to load up the enumeration */ + udev_enumerate_scan_devices(enumerate); + + /* Get a list we can walk */ + devices = udev_enumerate_get_list_entry(enumerate); + + /* 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)); + udev_device_unref(dev); + + count++; + if (count > names_len) + break; + } + + udev_enumerate_unref(enumerate); + + udev_unref(udev); + + return count; +} + +static virInterfacePtr +udevIfaceLookupByName(virConnectPtr conn, const char *name) +{ + struct udev_iface_driver *driverState = conn->interfacePrivateData; + struct udev *udev = udev_ref(driverState->udev); + struct udev_device *dev; + const char *macaddr; + virInterfacePtr ret; + + /* get a device reference based on the device name */ + dev = udev_device_new_from_subsystem_sysname(udev, "net", name); + if (!dev) { + virReportError(VIR_ERR_NO_INTERFACE, + _("couldn't find interface named '%s'"), + name); + return NULL; + } + + macaddr = udev_device_get_sysattr_value(dev, "address"); + ret = virGetInterface(conn, name, macaddr); + udev_device_unref(dev); + + udev_unref(udev); + + return ret; +} + +static virInterfacePtr +udevIfaceLookupByMACString(virConnectPtr conn, const char *macstr) +{ + struct udev_iface_driver *driverState = conn->interfacePrivateData; + struct udev *udev = udev_ref(driverState->udev); + struct udev_enumerate *enumerate = NULL; + struct udev_list_entry *dev_entry; + struct udev_device *dev; + const char *name; + virInterfacePtr ret; + + enumerate = udevIfaceGetDevices(udev, UDEV_IFACE_ALL); + + if (!enumerate) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to lookup interface with MAC address '%s'"), + macstr); + return NULL; + } + + /* Match on MAC */ + udev_enumerate_add_match_sysattr(enumerate, "address", macstr); + + /* Do the scan to load up the enumeration */ + udev_enumerate_scan_devices(enumerate); + + /* Get a list we can walk */ + dev_entry = udev_enumerate_get_list_entry(enumerate); + + /* Check that we got something back */ + if (!dev_entry) { + virReportError(VIR_ERR_NO_INTERFACE, + _("couldn't find interface with MAC address '%s'"), + macstr); + return NULL; + } + + /* Check that we didn't get multiple items back */ + if (udev_list_entry_get_next(dev_entry)) { + virReportError(VIR_ERR_MULTIPLE_INTERFACES, + _("the MAC address '%s' matches multiple interfaces"), + macstr); + return NULL; + } + + dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(dev_entry)); + name = udev_device_get_sysname(dev); + ret = virGetInterface(conn, name, macstr); + udev_device_unref(dev); + + udev_enumerate_unref(enumerate); + + udev_unref(udev); + + return ret; +} +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 */ +}; + +int +interfaceRegister(void) { + if (virRegisterInterfaceDriver(&udevIfaceDriver) < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to register udev interface driver")); + return 0; +} diff --git a/tools/virsh.c b/tools/virsh.c index 6a7b89d..7d682f2 100644 --- a/tools/virsh.c +++ 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 #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter"); -- 1.7.8.6
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
On 09/17/2012 07:27 PM, Doug Goldstein wrote:
Based exclusively on work by Eric Blake in a patch posted with the same subject. However some modifications related to comments and my plans to add another backend.
Added WITH_INTERFACE as the only automake variable deciding whether to build the driver and using WITH_NETCF to identify that we're wanting to use the netcf library as the backend.
* configure.ac: Added with_interface * src/interface/netcf_driver.c: Renamed.. * src/interface/interface_backend_netcf.c: ..to this to match storage. * src/interface/netcf_driver.h: Renamed.. * src/interface/interface_driver.h: ..to this. * daemon/Makefile.am: Respect WITH_INTERFACE and WITH_NETCF. * libvirt.spec.in: Add RPM support for --with-interface
'make syntax-check' says you missed a change to po/POTFILES.in.
---
Change from v2: * rebase against master * v2 conditionally ACK'd by Laine if Eric ACK'd it
Apologies for not having enough time to review this before rc1, but let's get it in for rc2.
+++ b/configure.ac @@ -1948,7 +1948,6 @@ AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS])
- AC_ARG_WITH([secrets],
Spurious whitespace change.
+++ b/libvirt.spec.in @@ -92,6 +92,7 @@ # A few optional bits off by default, we enable later %define with_polkit 0%{!?_without_polkit:0} %define with_capng 0%{!?_without_capng:0} +%define with_interface 0%{!?_without_interface:0} %define with_netcf 0%{!?_without_netcf:0} %define with_udev 0%{!?_without_udev:0} %define with_hal 0%{!?_without_hal:0} @@ -200,6 +201,12 @@ %define with_netcf 0%{!?_without_netcf:%{server_drivers}} %endif
+# interface is the driver that wraps netcf or udev interface management +# backends in Fedora 18 / RHEL-7 or newer
Not quite right. It wraps the netcf interface management in all versions of Fedora.
+%if 0%{?fedora} >= 18 || 0%{?rhel} >= 7 +%define with_interface 0%{!?_without_interface:%{server_drivers}} +%endif
So I think a better condition is to define with_interface to 1 if with_netcf is defined; then when you add a udev backend, define with_interface to 1 if either with_netcf or with_udev is defined.
-%if %{with_netcf} -%define with_interface 1 -%else -%define with_interface 0 -%endif
That is, this code already looked like it was doing the right thing. I think I can fix these things and push the amended version soon, but I ran out of time today. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Here's what I'm planning on squashing into Doug's patch; the biggest changes are to libvirt.spec.in, and I'd appreciate a review on that portion. --- configure.ac | 1 + libvirt.spec.in | 19 ++++++++++++------- po/POTFILES.in | 2 +- tools/virsh.c | 4 ++-- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/configure.ac b/configure.ac index 171dda5..3e90672 100644 --- a/configure.ac +++ b/configure.ac @@ -1948,6 +1948,7 @@ AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS]) + AC_ARG_WITH([secrets], AC_HELP_STRING([--with-secrets], [with local secrets management driver @<:@default=yes@:>@]),[],[with_secrets=yes]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 853cef7..1192739 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -70,6 +70,7 @@ %define with_parallels 0%{!?_without_parallels:1} # Then the secondary host drivers, which run inside libvirtd +%define with_interface 0%{!?_without_interface:%{server_drivers}} %define with_network 0%{!?_without_network:%{server_drivers}} %define with_storage_fs 0%{!?_without_storage_fs:%{server_drivers}} %define with_storage_lvm 0%{!?_without_storage_lvm:%{server_drivers}} @@ -92,7 +93,6 @@ # A few optional bits off by default, we enable later %define with_polkit 0%{!?_without_polkit:0} %define with_capng 0%{!?_without_capng:0} -%define with_interface 0%{!?_without_interface:0} %define with_netcf 0%{!?_without_netcf:0} %define with_udev 0%{!?_without_udev:0} %define with_hal 0%{!?_without_hal:0} @@ -201,12 +201,6 @@ %define with_netcf 0%{!?_without_netcf:%{server_drivers}} %endif -# interface is the driver that wraps netcf or udev interface management -# backends in Fedora 18 / RHEL-7 or newer -%if 0%{?fedora} >= 18 || 0%{?rhel} >= 7 -%define with_interface 0%{!?_without_interface:%{server_drivers}} -%endif - # udev is used to manage host devices in Fedora 12 / RHEL-6 or newer %if 0%{?fedora} >= 12 || 0%{?rhel} >= 6 %define with_udev 0%{!?_without_udev:%{server_drivers}} @@ -214,6 +208,11 @@ %define with_hal 0%{!?_without_hal:%{server_drivers}} %endif +# interface requires netcf +%if ! 0%{?with_netcf} +%define with_interface 0 +%endif + # Enable yajl library for JSON mode with QEMU %if 0%{?fedora} >= 13 || 0%{?rhel} >= 6 %define with_yajl 0%{!?_without_yajl:%{server_drivers}} @@ -233,6 +232,7 @@ # Disable some drivers when building without libvirt daemon. # The logic is the same as in configure.ac %if ! %{with_libvirtd} +%define with_interface 0 %define with_network 0 %define with_qemu 0 %define with_lxc 0 @@ -1114,6 +1114,10 @@ of recent versions of Linux (and other OSes). %define _with_rhel5_api --with-rhel5-api %endif +%if ! %{with_interface} +%define _without_interface --without-interface +%endif + %if ! %{with_network} %define _without_network --without-network %endif @@ -1250,6 +1254,7 @@ autoreconf -if %{?_without_hyperv} \ %{?_without_vmware} \ %{?_without_parallels} \ + %{?_without_interface} \ %{?_without_network} \ %{?_with_rhel5_api} \ %{?_without_storage_fs} \ diff --git a/po/POTFILES.in b/po/POTFILES.in index 7a91eb4..12a2b25 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -40,7 +40,7 @@ src/fdstream.c src/hyperv/hyperv_driver.c src/hyperv/hyperv_util.c src/hyperv/hyperv_wmi.c -src/interface/netcf_driver.c +src/interface/interface_backend_netcf.c src/internal.h src/libvirt.c src/libvirt-qemu.c diff --git a/tools/virsh.c b/tools/virsh.c index 6a7b89d..2c6df54 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2709,9 +2709,9 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #endif #if defined(WITH_INTERFACE) vshPrint(ctl, " Interface"); -#if defined(WITH_NETCF) +# if defined(WITH_NETCF) vshPrint(ctl, " netcf"); -#endif +# endif #endif #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter"); -- 1.7.11.4
On 09/19/2012 08:55 AM, Eric Blake wrote:
Here's what I'm planning on squashing into Doug's patch; the biggest changes are to libvirt.spec.in, and I'd appreciate a review on that portion.
--- configure.ac | 1 + libvirt.spec.in | 19 ++++++++++++------- po/POTFILES.in | 2 +- tools/virsh.c | 4 ++-- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/configure.ac b/configure.ac index 171dda5..3e90672 100644 --- a/configure.ac +++ b/configure.ac @@ -1948,6 +1948,7 @@ AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS])
+
What? Spurious whitespace changes from you? :-)
AC_ARG_WITH([secrets], AC_HELP_STRING([--with-secrets], [with local secrets management driver @<:@default=yes@:>@]),[],[with_secrets=yes])
diff --git a/libvirt.spec.in b/libvirt.spec.in index 853cef7..1192739 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -70,6 +70,7 @@ %define with_parallels 0%{!?_without_parallels:1}
# Then the secondary host drivers, which run inside libvirtd +%define with_interface 0%{!?_without_interface:%{server_drivers}} %define with_network 0%{!?_without_network:%{server_drivers}} %define with_storage_fs 0%{!?_without_storage_fs:%{server_drivers}} %define with_storage_lvm 0%{!?_without_storage_lvm:%{server_drivers}} @@ -92,7 +93,6 @@ # A few optional bits off by default, we enable later %define with_polkit 0%{!?_without_polkit:0} %define with_capng 0%{!?_without_capng:0} -%define with_interface 0%{!?_without_interface:0} %define with_netcf 0%{!?_without_netcf:0} %define with_udev 0%{!?_without_udev:0} %define with_hal 0%{!?_without_hal:0} @@ -201,12 +201,6 @@ %define with_netcf 0%{!?_without_netcf:%{server_drivers}} %endif
-# interface is the driver that wraps netcf or udev interface management -# backends in Fedora 18 / RHEL-7 or newer -%if 0%{?fedora} >= 18 || 0%{?rhel} >= 7 -%define with_interface 0%{!?_without_interface:%{server_drivers}} -%endif -
Right - whether or not with_interface is on has nothing to do with whether or not we're on fedora 18+ or RHEL7+
# udev is used to manage host devices in Fedora 12 / RHEL-6 or newer %if 0%{?fedora} >= 12 || 0%{?rhel} >= 6 %define with_udev 0%{!?_without_udev:%{server_drivers}} @@ -214,6 +208,11 @@ %define with_hal 0%{!?_without_hal:%{server_drivers}} %endif
+# interface requires netcf +%if ! 0%{?with_netcf} +%define with_interface 0 +%endif + # Enable yajl library for JSON mode with QEMU %if 0%{?fedora} >= 13 || 0%{?rhel} >= 6 %define with_yajl 0%{!?_without_yajl:%{server_drivers}} @@ -233,6 +232,7 @@ # Disable some drivers when building without libvirt daemon. # The logic is the same as in configure.ac %if ! %{with_libvirtd} +%define with_interface 0
Right - it should only have an effect on code that's linked into libvirtd anyway.
%define with_network 0 %define with_qemu 0 %define with_lxc 0 @@ -1114,6 +1114,10 @@ of recent versions of Linux (and other OSes). %define _with_rhel5_api --with-rhel5-api %endif
+%if ! %{with_interface} +%define _without_interface --without-interface +%endif +
Explicitly adding --without-interface when the specfile reaches that conclusion.
%if ! %{with_network} %define _without_network --without-network %endif @@ -1250,6 +1254,7 @@ autoreconf -if %{?_without_hyperv} \ %{?_without_vmware} \ %{?_without_parallels} \ + %{?_without_interface} \ %{?_without_network} \ %{?_with_rhel5_api} \ %{?_without_storage_fs} \ diff --git a/po/POTFILES.in b/po/POTFILES.in index 7a91eb4..12a2b25 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -40,7 +40,7 @@ src/fdstream.c src/hyperv/hyperv_driver.c src/hyperv/hyperv_util.c src/hyperv/hyperv_wmi.c -src/interface/netcf_driver.c +src/interface/interface_backend_netcf.c src/internal.h src/libvirt.c src/libvirt-qemu.c diff --git a/tools/virsh.c b/tools/virsh.c index 6a7b89d..2c6df54 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2709,9 +2709,9 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #endif #if defined(WITH_INTERFACE) vshPrint(ctl, " Interface"); -#if defined(WITH_NETCF) +# if defined(WITH_NETCF) vshPrint(ctl, " netcf"); -#endif +# endif #endif #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter");
ACK.
On 09/19/2012 07:41 AM, Laine Stump wrote:
On 09/19/2012 08:55 AM, Eric Blake wrote:
Here's what I'm planning on squashing into Doug's patch; the biggest changes are to libvirt.spec.in, and I'd appreciate a review on that portion.
--- configure.ac | 1 + libvirt.spec.in | 19 ++++++++++++------- po/POTFILES.in | 2 +- tools/virsh.c | 4 ++-- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/configure.ac b/configure.ac index 171dda5..3e90672 100644 --- a/configure.ac +++ b/configure.ac @@ -1948,6 +1948,7 @@ AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS])
+
What? Spurious whitespace changes from you? :-)
Rather, this is designed for squashing in to Doug's patch, to undo a spurious whitespace change :)
ACK.
Okay, I've squashed the two together, and pushed the result. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Sep 19, 2012 at 9:35 AM, Eric Blake <eblake@redhat.com> wrote:
On 09/19/2012 07:41 AM, Laine Stump wrote:
On 09/19/2012 08:55 AM, Eric Blake wrote:
Here's what I'm planning on squashing into Doug's patch; the biggest changes are to libvirt.spec.in, and I'd appreciate a review on that portion.
--- configure.ac | 1 + libvirt.spec.in | 19 ++++++++++++------- po/POTFILES.in | 2 +- tools/virsh.c | 4 ++-- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/configure.ac b/configure.ac index 171dda5..3e90672 100644 --- a/configure.ac +++ b/configure.ac @@ -1948,6 +1948,7 @@ AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS])
+
What? Spurious whitespace changes from you? :-)
Rather, this is designed for squashing in to Doug's patch, to undo a spurious whitespace change :)
Yes I was evil and had a white space change. I'll accept my public flogging.
ACK.
Okay, I've squashed the two together, and pushed the result.
--
Thank you much guys. -- Doug Goldstein
participants (4)
-
Doug Goldstein -
Doug Goldstein -
Eric Blake -
Laine Stump