Re: [libvirt] [PATCH] build: define WITH_INTERFACE for the driver
On 08/28/2012 09:17 PM, Doug Goldstein wrote: > > Really just looking for feedback if this an acceptable update to the previous > work. My plan is to add a 'udev' backend that will provide just some basic > read-only host interface information. Basically I've been seeing postings to > libvirt-users and virt-tools where people are on distros that don't support > netcf and are noticing that virt-manager uses HAL or they use libvir APIs and > can configure everything for the virtual machine respecting the host except > for the network. > It's best to stick comments like this... > > 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. Thanks for picking up work on my old attempt. > > 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 and enhanced with_netcf to respect > with_interface. This sounds backwards. If I recall the discussion back on my old attempt, the consensus moved towards wanting all library checks first, and all driver checks last. But I'll have to look at the actual configure changes to see if they make sense. > * 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. > --- ...here, after the '---' marker. Then 'git am' will automatically discard them, while they will still be available as a review hint to the reader. > src/Makefile.am | 24 +- > src/interface/interface_backend_netcf.c | 663 > +++ > src/interface/interface_driver.h| 29 ++ > src/interface/netcf_driver.c| 663 > --- > src/interface/netcf_driver.h| 29 -- 'git config diff.renames true', then this will output a MUCH more compact patch. > +++ b/configure.ac > @@ -1963,9 +1963,6 @@ 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]) This delays the netcf probe... > > > AC_ARG_WITH([secrets], > @@ -2806,6 +2803,46 @@ 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 ...but this tries to make decisions based on the probe... > + > +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 If the interface driver is off disable netcf > +if test "$with_interface" = "no" ; then > + with_netcf=no > +fi > + > +dnl We only use netcf for the interface driver so only enable it then > +AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) > +AC_SUBST([NETCF_CFLAGS]) > +AC_SUBST([NETCF_LIBS]) ...that doesn't occur until here. So this logic needs to be rewritten. I think the correct order is to check netcf first, (add in your new backend check at this point), and _finally_ check with_interface, using: if test $with_interface = yes; then require at least one backend (for now, netcf) is also yes, or fail else if test $with_interface = check; then if at least one backend is yes, then set yes fi > @@ -3018,6 +3055,7 @@ 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([]) I think there's still more data we should be outputting in this section, as well as some re-ordering. with_netcf should be output in the libraries section, not the drivers section. > +++ b/src/util/util.c > @@ -1251,39 +
[libvirt] [PATCH] build: define WITH_INTERFACE for the driver
Really just looking for feedback if this an acceptable update to the previous work. My plan is to add a 'udev' backend that will provide just some basic read-only host interface information. Basically I've been seeing postings to libvirt-users and virt-tools where people are on distros that don't support netcf and are noticing that virt-manager uses HAL or they use libvir APIs and can configure everything for the virtual machine respecting the host except for the network. 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 and enhanced with_netcf to respect 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. --- configure.ac| 44 ++- daemon/Makefile.am |2 +- daemon/libvirtd.c |8 +- src/Makefile.am | 24 +- src/interface/interface_backend_netcf.c | 663 +++ src/interface/interface_driver.h| 29 ++ src/interface/netcf_driver.c| 663 --- src/interface/netcf_driver.h| 29 -- src/util/util.c | 21 +- tests/virdrivermoduletest.c |2 +- tools/virsh.c |4 +- 11 files changed, 776 insertions(+), 713 deletions(-) create mode 100644 src/interface/interface_backend_netcf.c create mode 100644 src/interface/interface_driver.h delete mode 100644 src/interface/netcf_driver.c delete mode 100644 src/interface/netcf_driver.h diff --git a/configure.ac b/configure.ac index df39df2..2d7a55e 100644 --- a/configure.ac +++ b/configure.ac @@ -1963,9 +1963,6 @@ 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]) AC_ARG_WITH([secrets], @@ -2806,6 +2803,46 @@ 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 If the interface driver is off disable netcf +if test "$with_interface" = "no" ; then + with_netcf=no +fi + +dnl We only use netcf for the interface driver so only enable it then +AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) +AC_SUBST([NETCF_CFLAGS]) +AC_SUBST([NETCF_LIBS]) + dnl libblkid is used by several storage drivers; therefore we probe dnl for it unconditionally. AC_ARG_WITH([libblkid], @@ -3018,6 +3055,7 @@ 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 b00fc13..fbd5a5e 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -147,7 +147,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 19dd26b..c2923d5 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -75,8 +75,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" @@ -379,7 +3
Re: [libvirt] [PATCH] build: define WITH_INTERFACE for the driver
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: define WITH_INTERFACE for the driver
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-
Re: [libvirt] [PATCH] build: define WITH_INTERFACE for the driver
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}
Re: [libvirt] [PATCH] build: define WITH_INTERFACE for the driver
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
[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_N
