On Tue, Apr 19, 2016 at 2:34 AM, Laine Stump <[email protected]> wrote:
> On 04/18/2016 06:52 PM, Cole Robinson wrote: > >> On 04/15/2016 08:18 PM, Alberto Ruiz wrote: >> >>> From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001 >>> From: Alberto Ruiz <[email protected]> >>> Date: Wed, 13 Apr 2016 17:00:45 +0100 >>> Subject: [PATCH] network: Add support for dhcp-range lease time in the >>> network >>> XML configuration format and dnsmasq >>> >>> Also mention the bug in the commit message, just link it like >> >> https://bugzilla.redhat.com/show_bug.cgi?id=913446 >> >> Needs documentation but that will be dependent on what the final patch >> looks >> like, so fine to skip for now. >> >> The main questions are: >> >> 1) is the XML format fine? <range ... lease='XXX'/>. lease sounds kinda >> non-specific to me, maybe leasetime or leaseTime. >> >> 2) what to use for the input format? right now it's just string >> passthrough to >> dnsmasq, which takes a format like XX[s|m|h|d|w], or 'infinite'. Accepting >> that format kind of sticks us with that for all time, which probably >> isn't a >> good precedent. the easy way would probably be to just say the value >> needs to >> be in minutes, and maybe -1 == infinite. But that will take a bit more >> code to >> adapt that value to the dnsmasq format. >> > > 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. Agreed. Will work on a second version of this patch with taking this into account. > > > >> CC laining for his thoughts >> > > 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> > > Sounds fair, and solves another issue I was hoping to discuss which is per-host leasetime. > 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! No worries, my pleasure, will update the patch and get back to you as soon as I can. > And one tiny comment below: >> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >>> index 4fb2e2a..449c9ed 100644 >>> --- a/src/conf/network_conf.c >>> +++ b/src/conf/network_conf.c >>> @@ -313,6 +313,10 @@ static void >>> virNetworkIpDefClear(virNetworkIpDefPtr def) >>> { >>> VIR_FREE(def->family); >>> + >>> + while (def->nranges) >>> + VIR_FREE(def->ranges[--def->nranges].lease); >>> + >>> VIR_FREE(def->ranges); >>> while (def->nhosts) >>> @@ -855,7 +859,6 @@ int virNetworkIpDefNetmask(const virNetworkIpDef >>> *def, >>> >>> VIR_SOCKET_ADDR_FAMILY(&def->address)); >>> } >>> - >>> >> stray whitespace change here >> >> - Cole >> >> > -- Alberto Ruiz Associate Engineering Manager - Desktop Management Tools Red Hat
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
