[libvirt] [PATCH] build: define WITH_INTERFACE for the driver
Our code was mistakenly relying on an undefined macro, WITH_INTERFACE, for determining whether to load the interface driver which wraps the netcf library. Clean this situation up by having only one automake conditional for the driver, and having both WITH_NETCF (library detected) and WITH_INTERFACE (driver enabled) in C code, in case a future patch ever adds a network management via means other than the netcf library. While at it, output more information at the conclusion of configure about the various drivers we enabled. * configure.ac: Enhance with_netcf, and add with_interface. Improve output to list final decisions. Replace WITH_NETCF with WITH_INTERFACE. * src/interface/netcf_driver.c: Rename... * src/interface/interface_driver.c: ...to this. * src/interface/interface_driver.h: Likewise. * daemon/Makefile.am (libvirtd_LDADD): Reflect better naming. * src/Makefile.am (libvirt_driver_interface_la_*): Likewise. (INTERFACE_DRIVER_SOURCES): Reflect file moves. * daemon/libvirtd.c (daemonInitialize): Likewise. * tools/virsh.c (vshShowVersion): Show both driver and library decisions. * libvirt.spec.in (with_interface): Tweak to deal with new usage as a real switch. --- I think this addresses the point that Osier raised here: https://www.redhat.com/archives/libvir-list/2012-June/msg01266.html but it is complex enough that I'd appreciate a careful review. configure.ac | 44 ++++++++++++++++---- daemon/Makefile.am | 2 +- daemon/libvirtd.c | 6 +-- libvirt.spec.in | 10 +++-- src/Makefile.am | 4 +- .../{netcf_driver.c => interface_driver.c} | 4 +- .../{netcf_driver.h => interface_driver.h} | 0 tools/virsh.c | 11 +++-- 8 files changed, 59 insertions(+), 22 deletions(-) rename src/interface/{netcf_driver.c => interface_driver.c} (99%) rename src/interface/{netcf_driver.h => interface_driver.h} (100%) diff --git a/configure.ac b/configure.ac index 6436885..a29b3b2 100644 --- a/configure.ac +++ b/configure.ac @@ -1755,6 +1755,7 @@ if test "$with_network" = "yes" ; then fi AM_CONDITIONAL([WITH_NETWORK], [test "$with_network" = "yes"]) +dnl check whether helper code is needed for above selections with_bridge=no if test "$with_qemu:$with_lxc:$with_network" != "no:no:no"; then with_bridge=yes @@ -1762,16 +1763,31 @@ if test "$with_qemu:$with_lxc:$with_network" != "no:no:no"; then fi AM_CONDITIONAL([WITH_BRIDGE], [test "$with_bridge" = "yes"]) -dnl netcf library +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 there's no use compiling the interface driver without the libvirt daemon +if test "$with_libvirtd" = "no"; then + with_interface=no +fi + +dnl The interface driver depends on the netcf library AC_ARG_WITH([netcf], AC_HELP_STRING([--with-netcf], [libnetcf support to configure physical host network interfaces @<:@default=check@:>@]), [], [with_netcf=check]) NETCF_CFLAGS= NETCF_LIBS= -if test "$with_libvirtd" = "no" ; then +if test "$with_libvirtd" = "no" || test "$with_interface" = "no"; then with_netcf=no fi +if test "$with_interface:$with_netcf" = "yes:check"; then + with_netcf=yes +fi if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then PKG_CHECK_MODULES(NETCF, netcf >= $NETCF_REQUIRED, [with_netcf=yes], [ @@ -1792,11 +1808,21 @@ if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then fi fi fi -AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS]) +dnl Final decision on the interface driver +if test "$with_interface" = "check"; then + with_interface=$with_netcf +fi + +if test "$with_interface" = "yes" ; then + AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1], + [whether interface driver is enabled]) +fi +AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"]) +dnl Check whether the Secrets driver is needed AC_ARG_WITH([secrets], AC_HELP_STRING([--with-secrets], [with local secrets management driver @<:@default=yes@:>@]),[],[with_secrets=yes]) @@ -2807,11 +2833,12 @@ AC_MSG_NOTICE([ ESX: $with_esx]) AC_MSG_NOTICE([ Hyper-V: $with_hyperv]) 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([ macvtap: $with_macvtap]) -AC_MSG_NOTICE([virtport: $with_virtualport]) +AC_MSG_NOTICE([ Network: $with_network]) +AC_MSG_NOTICE([ Iface: $with_interface]) +AC_MSG_NOTICE([ Secrets: $with_secrets]) +AC_MSG_NOTICE([ NodeDev: $with_nodedev]) +AC_MSG_NOTICE([NWfilter: $with_nwfilter]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Storage Drivers]) AC_MSG_NOTICE([]) @@ -2977,6 +3004,9 @@ AC_MSG_NOTICE([ Readline: $lv_use_readline]) AC_MSG_NOTICE([ Python: $with_python]) AC_MSG_NOTICE([ DTrace: $with_dtrace]) AC_MSG_NOTICE([ numad: $with_numad]) +AC_MSG_NOTICE([ bridge: $with_bridge]) +AC_MSG_NOTICE([ macvtap: $with_macvtap]) +AC_MSG_NOTICE([ virtport: $with_virtualport]) AC_MSG_NOTICE([ XML Catalog: $XML_CATALOG_FILE]) AC_MSG_NOTICE([ Init script: $with_init_script]) AC_MSG_NOTICE([Console locks: $with_console_lock_files]) diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 71e91cd..f0e422e 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -148,7 +148,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 9c06344..a4f98cf 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -74,8 +74,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" @@ -400,7 +400,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 896ef51..26ea2d9 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -70,6 +70,7 @@ # Then the secondary host drivers, which run inside libvirtd %define with_network 0%{!?_without_network:%{server_drivers}} +%define with_interface 0%{!?_without_interface:%{server_drivers}} %define with_storage_fs 0%{!?_without_storage_fs:%{server_drivers}} %define with_storage_lvm 0%{!?_without_storage_lvm:%{server_drivers}} %define with_storage_iscsi 0%{!?_without_storage_iscsi:%{server_drivers}} @@ -214,6 +215,7 @@ # The logic is the same as in configure.ac %if ! %{with_libvirtd} %define with_network 0 +%define with_interface 0 %define with_qemu 0 %define with_lxc 0 %define with_uml 0 @@ -267,9 +269,7 @@ %define with_nodedev 0 %endif -%if %{with_netcf} -%define with_interface 1 -%else +%if !%{with_netcf} %define with_interface 0 %endif @@ -1056,6 +1056,9 @@ of recent versions of Linux (and other OSes). %define _without_network --without-network %endif +%if ! %{with_interface} +%define _without_interface --without-interface +%endif %if ! %{with_storage_fs} %define _without_storage_fs --without-storage-fs %endif @@ -1171,6 +1174,7 @@ autoreconf -if %{?_without_hyperv} \ %{?_without_vmware} \ %{?_without_network} \ + %{?_without_interface} \ %{?_with_rhel5_api} \ %{?_without_storage_fs} \ %{?_without_storage_lvm} \ diff --git a/src/Makefile.am b/src/Makefile.am index 2309984..3cfaf01 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -483,7 +483,7 @@ NETWORK_DRIVER_SOURCES = \ network/bridge_driver.h network/bridge_driver.c INTERFACE_DRIVER_SOURCES = \ - interface/netcf_driver.h interface/netcf_driver.c + interface/interface_driver.h interface/interface_driver.c SECRET_DRIVER_SOURCES = \ secret/secret_driver.h secret/secret_driver.c @@ -924,7 +924,7 @@ EXTRA_DIST += network/default.xml -if WITH_NETCF +if WITH_INTERFACE if WITH_DRIVER_MODULES mod_LTLIBRARIES += libvirt_driver_interface.la else diff --git a/src/interface/netcf_driver.c b/src/interface/interface_driver.c similarity index 99% rename from src/interface/netcf_driver.c rename to src/interface/interface_driver.c index 45e6442..4959c72 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/interface_driver.c @@ -2,7 +2,7 @@ * interface_driver.c: backend driver methods to handle physical * interface configuration using the netcf library. * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -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" 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/tools/virsh.c b/tools/virsh.c index 53d1825..c1e7010 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -20832,15 +20832,18 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #ifdef WITH_NETWORK vshPrint(ctl, " Network"); #endif -#ifdef WITH_BRIDGE - vshPrint(ctl, " Bridging"); -#endif -#ifdef WITH_NETCF +#ifdef WITH_INTERFACE vshPrint(ctl, " Interface"); #endif #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter"); #endif +#ifdef WITH_BRIDGE + vshPrint(ctl, " Bridging"); +#endif +#ifdef WITH_NETCF + vshPrint(ctl, " Netcf"); +#endif #ifdef WITH_VIRTUALPORT vshPrint(ctl, " VirtualPort"); #endif -- 1.7.10.4
On 2012年06月29日 07:57, Eric Blake wrote:
Our code was mistakenly relying on an undefined macro, WITH_INTERFACE, for determining whether to load the interface driver which wraps the netcf library. Clean this situation up by having only one automake conditional for the driver, and having both WITH_NETCF (library detected) and WITH_INTERFACE (driver enabled) in C code, in case a future patch ever adds a network management via means other than the netcf library.
Foresighted. :-)
While at it, output more information at the conclusion of configure about the various drivers we enabled.
* configure.ac: Enhance with_netcf, and add with_interface. Improve output to list final decisions. Replace WITH_NETCF with WITH_INTERFACE. * src/interface/netcf_driver.c: Rename... * src/interface/interface_driver.c: ...to this. * src/interface/interface_driver.h: Likewise. * daemon/Makefile.am (libvirtd_LDADD): Reflect better naming. * src/Makefile.am (libvirt_driver_interface_la_*): Likewise. (INTERFACE_DRIVER_SOURCES): Reflect file moves. * daemon/libvirtd.c (daemonInitialize): Likewise. * tools/virsh.c (vshShowVersion): Show both driver and library decisions. * libvirt.spec.in (with_interface): Tweak to deal with new usage as a real switch. ---
I think this addresses the point that Osier raised here: https://www.redhat.com/archives/libvir-list/2012-June/msg01266.html
but it is complex enough that I'd appreciate a careful review.
configure.ac | 44 ++++++++++++++++---- daemon/Makefile.am | 2 +- daemon/libvirtd.c | 6 +-- libvirt.spec.in | 10 +++-- src/Makefile.am | 4 +- .../{netcf_driver.c => interface_driver.c} | 4 +- .../{netcf_driver.h => interface_driver.h} | 0 tools/virsh.c | 11 +++-- 8 files changed, 59 insertions(+), 22 deletions(-) rename src/interface/{netcf_driver.c => interface_driver.c} (99%) rename src/interface/{netcf_driver.h => interface_driver.h} (100%)
diff --git a/configure.ac b/configure.ac index 6436885..a29b3b2 100644 --- a/configure.ac +++ b/configure.ac @@ -1755,6 +1755,7 @@ if test "$with_network" = "yes" ; then fi AM_CONDITIONAL([WITH_NETWORK], [test "$with_network" = "yes"])
+dnl check whether helper code is needed for above selections with_bridge=no if test "$with_qemu:$with_lxc:$with_network" != "no:no:no"; then with_bridge=yes @@ -1762,16 +1763,31 @@ if test "$with_qemu:$with_lxc:$with_network" != "no:no:no"; then fi AM_CONDITIONAL([WITH_BRIDGE], [test "$with_bridge" = "yes"])
-dnl netcf library +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])
Do we have to expose "with-interface"? It will give the user a logic question, pick "with-interface", or 'with-netcf', or both, even more when we have other implementations of interface driver in future. however, the logic is simple, and we do it inside actually: as long as one implementation of the interface driver is picked to compile, we have the WITH_INTERFACE. so IMHO no need to give the user the simple logic question. :-)
+ +dnl there's no use compiling the interface driver without the libvirt daemon +if test "$with_libvirtd" = "no"; then + with_interface=no +fi
If we don't expose 'with-interface', we don't need this......
+ +dnl The interface driver depends on the netcf library AC_ARG_WITH([netcf], AC_HELP_STRING([--with-netcf], [libnetcf support to configure physical host network interfaces @<:@default=check@:>@]), [], [with_netcf=check])
NETCF_CFLAGS= NETCF_LIBS= -if test "$with_libvirtd" = "no" ; then +if test "$with_libvirtd" = "no" || test "$with_interface" = "no"; then with_netcf=no fi +if test "$with_interface:$with_netcf" = "yes:check"; then + with_netcf=yes +fi if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then PKG_CHECK_MODULES(NETCF, netcf>= $NETCF_REQUIRED, [with_netcf=yes], [ @@ -1792,11 +1808,21 @@ if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then fi fi fi -AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) AC_SUBST([NETCF_CFLAGS]) AC_SUBST([NETCF_LIBS])
+dnl Final decision on the interface driver +if test "$with_interface" = "check"; then + with_interface=$with_netcf +fi + +if test "$with_interface" = "yes" ; then + AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1], + [whether interface driver is enabled]) +fi +AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"])
And above changes, what we need is just: if test "$with_netfs" = "yes" || "with_something_else" = "yes"; then AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1], [whether interface driver is enabled]) fi AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"])
+dnl Check whether the Secrets driver is needed AC_ARG_WITH([secrets], AC_HELP_STRING([--with-secrets], [with local secrets management driver @<:@default=yes@:>@]),[],[with_secrets=yes])
@@ -2807,11 +2833,12 @@ AC_MSG_NOTICE([ ESX: $with_esx]) AC_MSG_NOTICE([ Hyper-V: $with_hyperv]) 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])
And no AC_MSG_NOTICE for $with_interface here, with keeping $with_netcf.
-AC_MSG_NOTICE([ macvtap: $with_macvtap]) -AC_MSG_NOTICE([virtport: $with_virtualport]) +AC_MSG_NOTICE([ Network: $with_network]) +AC_MSG_NOTICE([ Iface: $with_interface]) +AC_MSG_NOTICE([ Secrets: $with_secrets]) +AC_MSG_NOTICE([ NodeDev: $with_nodedev]) +AC_MSG_NOTICE([NWfilter: $with_nwfilter]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Storage Drivers]) AC_MSG_NOTICE([]) @@ -2977,6 +3004,9 @@ AC_MSG_NOTICE([ Readline: $lv_use_readline]) AC_MSG_NOTICE([ Python: $with_python]) AC_MSG_NOTICE([ DTrace: $with_dtrace]) AC_MSG_NOTICE([ numad: $with_numad]) +AC_MSG_NOTICE([ bridge: $with_bridge]) +AC_MSG_NOTICE([ macvtap: $with_macvtap]) +AC_MSG_NOTICE([ virtport: $with_virtualport]) AC_MSG_NOTICE([ XML Catalog: $XML_CATALOG_FILE]) AC_MSG_NOTICE([ Init script: $with_init_script]) AC_MSG_NOTICE([Console locks: $with_console_lock_files]) diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 71e91cd..f0e422e 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -148,7 +148,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 9c06344..a4f98cf 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -74,8 +74,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" @@ -400,7 +400,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 896ef51..26ea2d9 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -70,6 +70,7 @@
# Then the secondary host drivers, which run inside libvirtd %define with_network 0%{!?_without_network:%{server_drivers}} +%define with_interface 0%{!?_without_interface:%{server_drivers}} %define with_storage_fs 0%{!?_without_storage_fs:%{server_drivers}} %define with_storage_lvm 0%{!?_without_storage_lvm:%{server_drivers}} %define with_storage_iscsi 0%{!?_without_storage_iscsi:%{server_drivers}} @@ -214,6 +215,7 @@ # The logic is the same as in configure.ac %if ! %{with_libvirtd} %define with_network 0 +%define with_interface 0 %define with_qemu 0 %define with_lxc 0 %define with_uml 0 @@ -267,9 +269,7 @@ %define with_nodedev 0 %endif
-%if %{with_netcf} -%define with_interface 1 -%else +%if !%{with_netcf} %define with_interface 0 %endif
@@ -1056,6 +1056,9 @@ of recent versions of Linux (and other OSes). %define _without_network --without-network %endif
+%if ! %{with_interface} +%define _without_interface --without-interface +%endif %if ! %{with_storage_fs} %define _without_storage_fs --without-storage-fs %endif @@ -1171,6 +1174,7 @@ autoreconf -if %{?_without_hyperv} \ %{?_without_vmware} \ %{?_without_network} \ + %{?_without_interface} \ %{?_with_rhel5_api} \ %{?_without_storage_fs} \ %{?_without_storage_lvm} \
So, no these changes either.
diff --git a/src/Makefile.am b/src/Makefile.am index 2309984..3cfaf01 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -483,7 +483,7 @@ NETWORK_DRIVER_SOURCES = \ network/bridge_driver.h network/bridge_driver.c
INTERFACE_DRIVER_SOURCES = \ - interface/netcf_driver.h interface/netcf_driver.c + interface/interface_driver.h interface/interface_driver.c
SECRET_DRIVER_SOURCES = \ secret/secret_driver.h secret/secret_driver.c @@ -924,7 +924,7 @@ EXTRA_DIST += network/default.xml
-if WITH_NETCF +if WITH_INTERFACE if WITH_DRIVER_MODULES mod_LTLIBRARIES += libvirt_driver_interface.la else diff --git a/src/interface/netcf_driver.c b/src/interface/interface_driver.c similarity index 99% rename from src/interface/netcf_driver.c rename to src/interface/interface_driver.c
We might want to rename it again if there is new implementations in future. But it needs changes anyway, so it's fine.
index 45e6442..4959c72 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/interface_driver.c @@ -2,7 +2,7 @@ * interface_driver.c: backend driver methods to handle physical * interface configuration using the netcf library. * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -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"
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/tools/virsh.c b/tools/virsh.c index 53d1825..c1e7010 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -20832,15 +20832,18 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #ifdef WITH_NETWORK vshPrint(ctl, " Network"); #endif -#ifdef WITH_BRIDGE - vshPrint(ctl, " Bridging"); -#endif -#ifdef WITH_NETCF +#ifdef WITH_INTERFACE vshPrint(ctl, " Interface");
And no this.
#endif #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter"); #endif +#ifdef WITH_BRIDGE + vshPrint(ctl, " Bridging"); +#endif +#ifdef WITH_NETCF + vshPrint(ctl, " Netcf"); +#endif #ifdef WITH_VIRTUALPORT vshPrint(ctl, " VirtualPort"); #endif
Regards, Osier
On 06/29/2012 01:34 AM, Osier Yang wrote:
On 2012年06月29日 07:57, Eric Blake wrote:
Our code was mistakenly relying on an undefined macro, WITH_INTERFACE, for determining whether to load the interface driver which wraps the netcf library. Clean this situation up by having only one automake conditional for the driver, and having both WITH_NETCF (library detected) and WITH_INTERFACE (driver enabled) in C code, in case a future patch ever adds a network management via means other than
s/network/interface/
the netcf library.
Foresighted. :-)
Trying to model after the storage driver, and how it has several backends.
-dnl netcf library +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])
Do we have to expose "with-interface"? It will give the user a logic question, pick "with-interface", or 'with-netcf', or both, even more when we have other implementations of interface driver in future. however, the logic is simple, and we do it inside actually: as long as one implementation of the interface driver is picked to compile, we have the WITH_INTERFACE. so IMHO no need to give the user the simple logic question. :-)
Good point. Looking at how storage did it, we have: --with-storage-dir --with-storage-fs ... but no top-level --with-storage. That is, you get WITH_STORAGE if any of the --with-storage-backends ended up as yes. At first, I was worried about back-compat (old builds were used to --with-netcf, and I didn't want to break that), but the more I think about it, the more I think that it's okay to break naming conventions for something that is easier to explain. I see two possible solutions, then: 1. Assume that like the storage driver, the interface driver will eventually have multiple backends. Then we would have: --with-interface-netcf as a way to select the netcf backend in the interface driver, and WITH_INTERFACE would be automatic if at least one backend (in this case, netcf being the only backend) is found. 2. Save the complexity of multiple backends for the day when we actually have multiple backends, and for now just have a single configure option --with-interface. Either way, I would completely ditch --with-netcf, and refactor the logic to be: if test "$with_libvirtd" = no; then with_interface_netcf=no fi if test "$with_interface_netcf" = yes || \ test "$with_interface_netcf" = check; then probe for netcf, fail if it was required fi if test "$with_interface_netcf" = yes; then set WITH_INTERFACE witness fi I'll go ahead and respin this patch along those lines.
And above changes, what we need is just:
if test "$with_netfs" = "yes" || "with_something_else" = "yes"; then AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1], [whether interface driver is enabled]) fi AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"])
Yep, we're thinking on the same lines - probe for each backend, then make the driver decision based on the result of the backend probes, but only expose the backends as the configure options.
@@ -2807,11 +2833,12 @@ AC_MSG_NOTICE([ ESX: $with_esx]) AC_MSG_NOTICE([ Hyper-V: $with_hyperv]) 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])
And no AC_MSG_NOTICE for $with_interface here, with keeping $with_netcf.
-AC_MSG_NOTICE([ macvtap: $with_macvtap]) -AC_MSG_NOTICE([virtport: $with_virtualport]) +AC_MSG_NOTICE([ Network: $with_network]) +AC_MSG_NOTICE([ Iface: $with_interface]) +AC_MSG_NOTICE([ Secrets: $with_secrets]) +AC_MSG_NOTICE([ NodeDev: $with_nodedev]) +AC_MSG_NOTICE([NWfilter: $with_nwfilter])
Actually, no with_netcf here (this is the driver section, but with_netcf is already present in the library section), so we DO want a listing here of whether the with_interface driver was selected (whether by with_netcf or by some other backend).
-%if %{with_netcf} -%define with_interface 1 -%else +%if !%{with_netcf} %define with_interface 0 %endif
The logic here would be a bit different; just as the spec file has to know when to package the storage driver (if any of the storage backends were selected), we still have to package the interface driver, so this variable is still useful.
@@ -1056,6 +1056,9 @@ of recent versions of Linux (and other OSes). %define _without_network --without-network %endif
+%if ! %{with_interface} +%define _without_interface --without-interface +%endif %if ! %{with_storage_fs} %define _without_storage_fs --without-storage-fs %endif @@ -1171,6 +1174,7 @@ autoreconf -if %{?_without_hyperv} \ %{?_without_vmware} \ %{?_without_network} \ + %{?_without_interface} \ %{?_with_rhel5_api} \ %{?_without_storage_fs} \ %{?_without_storage_lvm} \
So, no these changes either.
Agreed - the netcf backend check should be sufficient here.
rename from src/interface/netcf_driver.c rename to src/interface/interface_driver.c
We might want to rename it again if there is new implementations in future. But it needs changes anyway, so it's fine.
The driver frontend should still be interface_driver.c. Rather, if we create a new backend, we would end up moving half the code into a new file interface_backend_netcf.c, as well as adding a new interface_backend_foo.c, where both backends are driven by interface_driver.c. But I agree that we should save that for a future day if we ever add another backend.
+++ b/tools/virsh.c @@ -20832,15 +20832,18 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #ifdef WITH_NETWORK vshPrint(ctl, " Network"); #endif -#ifdef WITH_BRIDGE - vshPrint(ctl, " Bridging"); -#endif -#ifdef WITH_NETCF +#ifdef WITH_INTERFACE vshPrint(ctl, " Interface");
And no this.
Actually, this is useful - we want to list all the drivers, as well as all the backends for each driver. 'Network', 'Interface', and 'Nwfilter' are drivers, while...
#endif #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter"); #endif +#ifdef WITH_BRIDGE + vshPrint(ctl, " Bridging"); +#endif +#ifdef WITH_NETCF + vshPrint(ctl, " Netcf"); +#endif #ifdef WITH_VIRTUALPORT vshPrint(ctl, " VirtualPort"); #endif
'Bridging', 'Netcf', and 'VirtualPort' are backends (not all for the same driver, but hopefully this is helping make the point). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Jun 29, 2012 at 10:18:47AM -0600, Eric Blake wrote:
On 06/29/2012 01:34 AM, Osier Yang wrote:
On 2012年06月29日 07:57, Eric Blake wrote:
Our code was mistakenly relying on an undefined macro, WITH_INTERFACE, for determining whether to load the interface driver which wraps the netcf library. Clean this situation up by having only one automake conditional for the driver, and having both WITH_NETCF (library detected) and WITH_INTERFACE (driver enabled) in C code, in case a future patch ever adds a network management via means other than
s/network/interface/
the netcf library.
Foresighted. :-)
Trying to model after the storage driver, and how it has several backends.
-dnl netcf library +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])
Do we have to expose "with-interface"? It will give the user a logic question, pick "with-interface", or 'with-netcf', or both, even more when we have other implementations of interface driver in future. however, the logic is simple, and we do it inside actually: as long as one implementation of the interface driver is picked to compile, we have the WITH_INTERFACE. so IMHO no need to give the user the simple logic question. :-)
Good point. Looking at how storage did it, we have:
--with-storage-dir --with-storage-fs ...
but no top-level --with-storage. That is, you get WITH_STORAGE if any of the --with-storage-backends ended up as yes.
At first, I was worried about back-compat (old builds were used to --with-netcf, and I didn't want to break that), but the more I think about it, the more I think that it's okay to break naming conventions for something that is easier to explain.
I see two possible solutions, then:
1. Assume that like the storage driver, the interface driver will eventually have multiple backends. Then we would have:
--with-interface-netcf
as a way to select the netcf backend in the interface driver, and WITH_INTERFACE would be automatic if at least one backend (in this case, netcf being the only backend) is found.
2. Save the complexity of multiple backends for the day when we actually have multiple backends, and for now just have a single configure option --with-interface.
Either way, I would completely ditch --with-netcf, and refactor the logic to be:
if test "$with_libvirtd" = no; then with_interface_netcf=no fi if test "$with_interface_netcf" = yes || \ test "$with_interface_netcf" = check; then probe for netcf, fail if it was required fi if test "$with_interface_netcf" = yes; then set WITH_INTERFACE witness fi
I'll go ahead and respin this patch along those lines.
I'm not a fan of this, because you are too tightly associating use of the netcf library, with use of the interface drivers, and also presuming a 1-1 relationship between a logical driver, and an external library. THis breaks down if a module like the inteface driver needs to check for multiple external libraries, and if the external libraries are used by multiple different areas of the libvirt code. My view is that in the configure script, we have two types of checks and we must keep them strictly separated. - External modules (netcf, lvm, other libraries) - Logical modules (storage driver, network driver, interface driver) We should first do checks for the external modules. These checks can be disabled/forced using --with-netcf/--without-netcf The checks for logical modules, should just look to see if their all of their prerequisites are present, but again allow you to turn off the module using --with-interface/--without-interface My long term vision is that we one day refactor our enourmous configure script into a set of isolated modules. So, you'd be able to declare logical modules AC_DEFUN(LIBVIRT_LIBRARY_NETCF, [ ...code to check for netcf and CLI args to enable/disable ]) AC_DEFUN(LIBVIRT_DRIVER_INTERFACE, [ AC_REQUIRE([LIBVIRT_DEP_NETCF]) ...code to enable interface driver if netcf was present ]) AC_DEFUN(LIBVIRT_DRIVER_STORAGE, [ AC_REQUIRE([LIBVIRT_DEP_LVM]) AC_REQUIRE([LIBVIRT_DEP_QEMU_IMG]) AC_REQUIRE([LIBVIRT_DEP_ISCSI]) ...code to enable storage driver parts... ]) and each of these definitions be completely separate .m4 files. So the eventual libvirt configure.ac script would just be doing LIBVIRT_DRIVER_INTERFACE LIBVIRT_DRIVER_STORAGE and so on. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 2012年06月30日 00:32, Daniel P. Berrange wrote:
On Fri, Jun 29, 2012 at 10:18:47AM -0600, Eric Blake wrote:
On 06/29/2012 01:34 AM, Osier Yang wrote:
On 2012年06月29日 07:57, Eric Blake wrote:
Our code was mistakenly relying on an undefined macro, WITH_INTERFACE, for determining whether to load the interface driver which wraps the netcf library. Clean this situation up by having only one automake conditional for the driver, and having both WITH_NETCF (library detected) and WITH_INTERFACE (driver enabled) in C code, in case a future patch ever adds a network management via means other than
s/network/interface/
the netcf library.
Foresighted. :-)
Trying to model after the storage driver, and how it has several backends.
-dnl netcf library +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])
Do we have to expose "with-interface"? It will give the user a logic question, pick "with-interface", or 'with-netcf', or both, even more when we have other implementations of interface driver in future. however, the logic is simple, and we do it inside actually: as long as one implementation of the interface driver is picked to compile, we have the WITH_INTERFACE. so IMHO no need to give the user the simple logic question. :-)
Good point. Looking at how storage did it, we have:
--with-storage-dir --with-storage-fs ...
but no top-level --with-storage. That is, you get WITH_STORAGE if any of the --with-storage-backends ended up as yes.
At first, I was worried about back-compat (old builds were used to --with-netcf, and I didn't want to break that), but the more I think about it, the more I think that it's okay to break naming conventions for something that is easier to explain.
I see two possible solutions, then:
1. Assume that like the storage driver, the interface driver will eventually have multiple backends. Then we would have:
--with-interface-netcf
as a way to select the netcf backend in the interface driver, and WITH_INTERFACE would be automatic if at least one backend (in this case, netcf being the only backend) is found.
2. Save the complexity of multiple backends for the day when we actually have multiple backends, and for now just have a single configure option --with-interface.
Either way, I would completely ditch --with-netcf, and refactor the logic to be:
if test "$with_libvirtd" = no; then with_interface_netcf=no fi if test "$with_interface_netcf" = yes || \ test "$with_interface_netcf" = check; then probe for netcf, fail if it was required fi if test "$with_interface_netcf" = yes; then set WITH_INTERFACE witness fi
I'll go ahead and respin this patch along those lines.
I'm not a fan of this, because you are too tightly associating use of the netcf library, with use of the interface drivers, and also presuming a 1-1 relationship between a logical driver, and an external library. THis breaks down if a module like the inteface driver needs to check for multiple external libraries, and if the external libraries are used by multiple different areas of the libvirt code.
Good point!
My view is that in the configure script, we have two types of checks and we must keep them strictly separated.
- External modules (netcf, lvm, other libraries) - Logical modules (storage driver, network driver, interface driver)
We should first do checks for the external modules. These checks can be disabled/forced using --with-netcf/--without-netcf
The checks for logical modules, should just look to see if their all of their prerequisites are present, but again allow you to turn off the module using --with-interface/--without-interface
My long term vision is that we one day refactor our enourmous configure script into a set of isolated modules.
So, you'd be able to declare logical modules
AC_DEFUN(LIBVIRT_LIBRARY_NETCF, [ ...code to check for netcf and CLI args to enable/disable ])
AC_DEFUN(LIBVIRT_DRIVER_INTERFACE, [ AC_REQUIRE([LIBVIRT_DEP_NETCF])
...code to enable interface driver if netcf was present ])
AC_DEFUN(LIBVIRT_DRIVER_STORAGE, [ AC_REQUIRE([LIBVIRT_DEP_LVM]) AC_REQUIRE([LIBVIRT_DEP_QEMU_IMG]) AC_REQUIRE([LIBVIRT_DEP_ISCSI])
...code to enable storage driver parts... ])
This model much more flexible and modularized. It seperates the definition of library and module. We will need this as current configure script is a bit disorderly. Regards, Osier
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang