On Mon, Apr 18, 2016 at 11:52 PM, Cole Robinson <crobi...@redhat.com> wrote:
> On 04/15/2016 08:18 PM, Alberto Ruiz wrote: > > From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001 > > From: Alberto Ruiz <ar...@gnome.org> > > 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. > Sounds good to me, though since pointed out in a later email, we might want to make this a child tag on its own within <dhcp> instead of a per range property. > 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. > Sticking to minutes seems like a loss of granularity, we can't really predict that a given setup might care about second level granularity for leases can we? I'm all for the argument of sanity checking the input and not doing an opaque passthrough, but I think in the end we might want to support some sort of seconds/minutes/hours/days notation (or stick to seconds and let people figure the amount out with a calculator). Thanks a lot for the input. > CC laining for his thoughts > > 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 libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
