[libvirt] [RFC/PATCH 1/2] network: add dnsmasq option 'dhcp-authoritative'

[Laine Stump]

On 09/21/2016 04:49 AM, Martin Wilck wrote:
> The dnsmasq man page recommends that dhcp-authoritative "should be
> set when dnsmasq is definitely the only DHCP server on a network".
> This is the case for libvirt-managed virtual networks.
>
> The effect of this is that VMs that fail to renew their DHCP lease
> in time (e.g. if the VM or host is suspended) will be able to
> re-acquire the lease even if it's expired, unless the IP address has
> been taken by some other host. This avoids various annoyances caused
> by changing VM IP addresses.

We had earlier (finally!) agreed in principle on pushing this patch, but 
were in freeze at the time, and I later forgot to do it.

I checked to be sure that the dnsmasq on one of the oldest Linux distros 
that we still support does have the dhcp-authoritative option. It does 
(it's dnsmasq-2.48-something) so we don't need any extra code to verify 
the option is there.

I do have a couple comments below, so don't hit the "next" button yet! :-)

> ---
>   src/network/bridge_driver.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 7b99aca..cb4fb1c 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1258,8 +1258,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>   
>           /* Note: the following is IPv4 only */
>           if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) {
> -            if (ipdef->nranges || ipdef->nhosts)
> +            if (ipdef->nranges || ipdef->nhosts) {
>                   virBufferAddLit(&configbuf, "dhcp-no-override\n");
> +                virBufferAddLit(&configbuf, "dhcp-authoritative\n");
> +	    }

You accidentally got a tab character in the line below, so I replaced it 
with spaces so that make syntax-check passes.

>   
>               if (ipdef->tftproot) {
>                   virBufferAddLit(&configbuf, "enable-tftp\n");


Although your cover letter included diff statistics for the 
networkxml2conftest test cases that needed the extra line for 
dhcp-authoritative, those diffs weren't included in the patch itself. 
Fortunately it's simple to regenerate the test case outputs 
(VIR_TEST_REGENERATE_OUTPUT=1 tests/networkxml2conftest), so I 
regenerated and verified them.


ACK, and pushed. Thanks for the submission, and sorry for the delay (and 
the seemingly protracted debate about a single line - we've been burned 
in the past by unforeseen consequences of adding a seemingly innocuous 
dnsmasq option to our dnsmasq conf files, and I was nervous about that 
happening again, so I wanted to make sure we explored all possibilities.)