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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list