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... ])