Re: [libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq

On 04/18/2016 06:52 PM, Cole Robinson wrote: Yeah, you should never just read an opaque string and pass it directly through to dnsmasq. Instead, parse an integer (and whatever scaling info - hours / minutes / seconds - I know we do that for bytes vs kbytes vs KiB etc, and if we don't already have the same thing for times somewhere, we should), scale it, check the range for some amount of sanity, and convert that scaled integer into whatever dnsmasq wants when building the commandline. Aside from the missing documentation that you pointed out (and that is just a pain to put in until the exact placement in the XML is figured out anyway), my main sticking point is having the lease time put as an attribute to each range. That just seems.... odd. I know that dnsmasq allows for specifying a lease time per range, but is that the case for other dhcp server implementations? (yeah, I know we don't support any other now, but someday we might :-). And even if dnsmasq *allows* it, unless you're using their tagging to put certain clients into certain IP ranges, there's no practical value in having a different lease time for each range. Maybe it should be an attribute of the <dhcp> element (or, to allow for different scaling, a subelement); every range on the dnsmasq commandline would just get the same lease time. Something like this: <dhcp> <leaseTime units='seconds'>3600</leasetime> <range blah blah blah/> .... </dhcp> If the need for per-range leasetime arose later, that could be added as a sub-element to <range> that would override the leasetime directly under <dhcp>. (It's been at least 15 years since I used ISC's dhcpd, but I glanced at the config file manpage just now and it looks like it's possible to have a single "max-lease" that applies to all "pools" (their name for ranges) or to specify a separate max-lease for each pool. I admit I skipped 98% of the contents though :-)). In practice, I doubt there will be much difference between what you proposed and what I've suggested - probably 100% of the libvirt virtual networks in existence have only a single range anyway. I *think* splitting it out from the range could prevent us from being painted into a corner though. Aside from all that, thanks for taking the time to code this up!