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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list