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

On 28/02/13 04:09, Eric Blake wrote: 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.