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

[Alberto Ruiz]

On Mon, Apr 18, 2016 at 11:52 PM, Cole Robinson <crobinso at 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 <aruiz at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160419/82dc6aa2/attachment-0001.htm>