[libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent

[TJ]

On 28/02/13 04:09, Eric Blake wrote:
> On 02/27/2013 07:57 PM, TJ wrote:
>> From: TJ <linux at iam.tj>
>>
>> A DHCP relay daemon will be started that will forward all DHCP/BOOTP
>> requests on the bridge network via the first declared forward
>> interface. The relay is broadcast rather than routed so no IP address
>> is needed on the bridge.
>>
> 
>> +static int
>> +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout,
>> +                                  char *pidfile)
>> +{
>> +    virCommandPtr cmd = NULL;
>> +    int ret = -1;
>> +
>> +    cmd = virCommandNew(DHCPRELAY);
> 
> Your patches are out of order.  This patch tries to use DHCPRELAY, but
> that isn't defined until patch 8/10 modifies configure.ac.  Squash those
> two patches together.  Each patch needs to be buildable in isolation.
>

One thing I'm not yet sure how to deal with is the difference between build and deployment systems when
one or the other doesn't have access to 'dhcp-helper', since as I understand it, some distributions do not
include it in their package archives (it is available in Debian/Ubuntu).

With the current implementation it must be available on the build system since otherwise the configure.ac definition of DHCPRELAY won't happen and it won't be declared in config.h.in, which means that
compilation of the source-code relying on it will fail. It seems messy but the only realistic way to deal with that is to surround the relevant code block(s) with

#ifdef DHCPRELAY
...
#endif

My preferred option is to change the way DHCPRELAY is handled entirely. I copied its definition style from DNSMASQ which, as far as I can see, *must* exist on the build system and in the same absolute
path as it will be found on the deployment system.

It might be better to simply declare the expected basename of the executable so instead of:

DHCPRELAY=/usr/sbin/dhcp-helper

we have

DHCPRELAY=dhcp-helper

then allow libvirt to find that executable using the PATH at run-time (which also accommodates user builds installed to /usr/local/sbin/). Taking that further to abstract the DHCPRELAY code entirely
from dhcp-helper (since unlike dnsmasq it isn't required to be available), it might make sense to add an optional 'agent' attribute to the <dhcp> element; something like <dhcp relay='yes'
agent='isc-relay'/> (which would find 'agent' on the PATH).

This would allow the default to be taken from DHCPRELAY but over-ridden by the optional 'agent' attribute.

That then leaves the issue of different DHCP relay agents requiring different command-line parameters. I was looking at the discussion about the <option> element and wondering if that could have a
dual use here, especially in the name= variety, e.g:

<ip ...>
 <dhcp relay='yes' agent='some-agent-we-dont-know'>
  <option name="bridge">
   <value data="-b"/>
  </option>
  <option name="interface">
   <value data="-i"/>
  </option>
  <option name="pidfile">
   <value data="-r"/>
  </option>
 </dhcp>
</ip>

For the DHCP relay the <option> names ("bridge", "interface", "pdifile") are constants that libvirt looks for in the XML. It replaces its hard-coded command-line switch defaults with the respective
<value data="..."> if found.

That approach would give the DHCP relay code the flexibility to evolve based on sysadmin needs without requiring any code modification.