Re: [libvirt] [PATCH] leasetime support for
Hey Laine, Have some time to review this? 2017-06-23 1:44 GMT+01:00 <ar...@gnome.org>: > From: Alberto Ruiz <ar...@gnome.org> > > Fixes #913446 > > This patch addresses a few problems found by the initial reviews: > > * leaseTimeUnit RNG type renamed to timeUnit > * virNetworkDHCPDefGetLeaseTime() renamed to virNetworkDHCPLeaseTimeParseXML() > * consistent use of braces in if-else-if > * use %lu instead of PRId64 > * use 0 as infinite lease > * add a leasetime_defined field to struct _virNetworkIPDef to describe > whether the value was set in the xml configuration or not > * use uint32_t for the leasetime instead of int64_t > * fail on all invalid leasetime values > * squash all patches into one > > --- > docs/schemas/basictypes.rng| 16 +++ > docs/schemas/network.rng | 8 ++ > src/conf/network_conf.c| 93 +++- > src/conf/network_conf.h| 5 +- > src/libvirt_private.syms | 1 + > src/network/bridge_driver.c| 119 > - > src/network/bridge_driver.h| 1 + > src/util/virdnsmasq.c | 106 +++--- > src/util/virdnsmasq.h | 2 + > .../dhcp6-nat-network.hostsfile| 7 ++ > tests/networkxml2confdata/dhcp6-network.hostsfile | 5 + > .../dhcp6host-routed-network.hostsfile | 7 ++ > tests/networkxml2confdata/leasetime-days.conf | 18 > tests/networkxml2confdata/leasetime-days.xml | 18 > tests/networkxml2confdata/leasetime-hours.conf | 18 > tests/networkxml2confdata/leasetime-hours.xml | 18 > tests/networkxml2confdata/leasetime-infinite.conf | 18 > tests/networkxml2confdata/leasetime-infinite.xml | 18 > tests/networkxml2confdata/leasetime-minutes.conf | 18 > tests/networkxml2confdata/leasetime-minutes.xml| 18 > tests/networkxml2confdata/leasetime-seconds.conf | 18 > tests/networkxml2confdata/leasetime-seconds.xml| 18 > tests/networkxml2confdata/leasetime.conf | 18 > tests/networkxml2confdata/leasetime.xml| 18 > .../nat-network-dns-srv-record-minimal.hostsfile | 2 + > .../nat-network-dns-srv-record.hostsfile | 2 + > .../nat-network-dns-txt-record.hostsfile | 2 + > .../nat-network-name-with-quotes.hostsfile | 2 + > tests/networkxml2confdata/nat-network.hostsfile| 2 + > .../networkxml2confdata/ptr-domains-auto.hostsfile | 2 + > tests/networkxml2conftest.c| 45 ++-- > 31 files changed, 571 insertions(+), 72 deletions(-) > create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile > create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile > create mode 100644 > tests/networkxml2confdata/dhcp6host-routed-network.hostsfile > create mode 100644 tests/networkxml2confdata/leasetime-days.conf > create mode 100644 tests/networkxml2confdata/leasetime-days.xml > create mode 100644 tests/networkxml2confdata/leasetime-hours.conf > create mode 100644 tests/networkxml2confdata/leasetime-hours.xml > create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf > create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml > create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf > create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml > create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf > create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml > create mode 100644 tests/networkxml2confdata/leasetime.conf > create mode 100644 tests/networkxml2confdata/leasetime.xml > create mode 100644 > tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile > create mode 100644 > tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile > create mode 100644 > tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile > create mode 100644 > tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile > create mode 100644 tests/networkxml2confdata/nat-network.hostsfile > create mode 100644 tests/networkxml2confdata/ptr-domains-auto.hostsfile > > diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng > index 1ea667cdf..9db19c7f0 100644 > --- a/docs/schemas/basictypes.rng > +++ b/docs/schemas/basictypes.rng > @@ -564,4 +564,20 @@ > > > > + > + > + seconds > + minutes > + hours >
Re: [libvirt] [PATCH 1/3] leasetime support for globally
2017-06-22 21:47 GMT+01:00 Laine Stump <la...@laine.org>: > On 06/22/2017 12:21 PM, Alberto Ruiz wrote: >> Hello Laine, >> >> 2017-06-21 17:30 GMT+01:00 Laine Stump <la...@laine.org >> <mailto:la...@laine.org>>: >> >> On 06/21/2017 03:27 AM, Peter Krempa wrote: >> > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org >> <mailto:ar...@gnome.org> wrote: >> >> From: Alberto Ruiz <ar...@gnome.org <mailto:ar...@gnome.org>> >> > >> > Missing commit message. >> >> And when composing the commit message, it's useful to include links to >> the associated BZ. >> >> https://bugzilla.redhat.com/show_bug.cgi?id=913446 >> <https://bugzilla.redhat.com/show_bug.cgi?id=913446> >> >> Also, I recall there being quite a lot of discussion in email (and >> possibly IRC) about the fact that people *think* they want a >> configurable lease time because they think that will eliminate cases of >> a DHCP lease being lost while a domain is paused. It was pointed out >> that lengthening the lease will *not* eliminate that problem (it just >> makes it happen less often). >> >> As an alternate (and better) solution to the problem of lost leases, we >> then added the "dhcp-authoritative" option to dnsmasq (commit >> 4ac20b3ae4), which allows clients to re-acquire the same IP as they had >> for an expired lease (as long as it hasn't been acquired by someone else >> in the meantime, which is apparently unlikely unless all the other >> addresses in the pool are already assigned). >> >> I'm not saying this to discourage the idea of making leasetime >> configurable (I think we'd already agreed that it was reasonable to do >> so, but there were two competing patches posted, and neither of them was >> really push-ready), but just to make sure that nobody is disappointed if >> the results don't lead to the behavior they're hoping for. >> >> >> My main motivation _was_ the loss of IP addresses after reboot on GNOME >> Boxes. >> >> However, given that I've written the patches already and some people >> might find this useful, I'm okay with going ahead and get them ready for >> approval. >> >> >> > >> >> >> >> --- >> >> docs/schemas/basictypes.rng | 16 + >> >> docs/schemas/network.rng | 8 +++ >> >> src/conf/network_conf.c | 78 >> ++- >> >> src/conf/network_conf.h | 3 +- >> >> src/network/bridge_driver.c | 49 >> +- >> >> tests/networkxml2confdata/leasetime-days.conf | 17 + >> >> tests/networkxml2confdata/leasetime-days.xml | 18 ++ >> >> tests/networkxml2confdata/leasetime-hours.conf| 17 + >> >> tests/networkxml2confdata/leasetime-hours.xml | 18 ++ >> >> tests/networkxml2confdata/leasetime-infinite.conf | 17 + >> >> tests/networkxml2confdata/leasetime-infinite.xml | 18 ++ >> >> tests/networkxml2confdata/leasetime-minutes.conf | 17 + >> >> tests/networkxml2confdata/leasetime-minutes.xml | 18 ++ >> >> tests/networkxml2confdata/leasetime-seconds.conf | 17 + >> >> tests/networkxml2confdata/leasetime-seconds.xml | 18 ++ >> >> tests/networkxml2confdata/leasetime.conf | 17 + >> >> tests/networkxml2confdata/leasetime.xml | 18 ++ >> >> tests/networkxml2conftest.c | 7 ++ >> >> 18 files changed, 368 insertions(+), 3 deletions(-) >> >> create mode 100644 tests/networkxml2confdata/leasetime-days.conf >> >> create mode 100644 tests/networkxml2confdata/leasetime-days.xml >> >> create mode 100644 tests/networkxml2confdata/leasetime-hours.conf >> >> create mode 100644 tests/networkxml2confdata/leasetime-hours.xml >> >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf >> >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml >> >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf >> >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml &g
Re: [libvirt] [PATCH 1/3] leasetime support for globally
2017-06-22 22:05 GMT+01:00 Laine Stump <la...@laine.org>:
>
> On 06/22/2017 01:12 PM, Alberto Ruiz wrote:
> >
> >
> > 2017-06-21 17:30 GMT+01:00 Laine Stump <la...@laine.org
> > <mailto:la...@laine.org>>:
> >
> > On 06/21/2017 03:27 AM, Peter Krempa wrote:
> > > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org
> > <mailto:ar...@gnome.org> wrote:
> > >> From: Alberto Ruiz <ar...@gnome.org <mailto:ar...@gnome.org>>
> > >
> > >> +
> > >> +if (result > UINT32_MAX) {
> > >> +virReportError(VIR_ERR_XML_ERROR,
> > >> + _(" value cannot be greater than
> > the equivalent of %" PRIo32 " seconds : %" PRId64),
> >
> > We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better
> > to "go with the flow" and just use "%d" instead for consistency (or make
> > a case for changing them elsewhere :-)
>
> (Is it possible for you to change the "quotation" character in your
> email client so that when you inline quote the original in a reply, it
> has "> " at the beginning of the line rather than " "? Using blanks
> as the quotation character makes it more likely to confuse who wrote
> which parts (especially when everything isn't starting in column 1))
I'll try to reply using plain text (using gmail here)
> > I knew I added this for something:
> >
> > This is the error I get when I just use "%d":
> >
> > conf/network_conf.c: In function 'virNetworkDHCPLeaseTimeParseXML':
> > conf/network_conf.c:575:26: error: format '%d' expects argument of type
> > 'int', but argument 8 has type 'int64_t {aka long int}' [-Werror=format=]
> > _(" value cannot be greater than the
> > equivalent of %d seconds : %d"),
> >
> > If you can think of any alternative that complies with libvirt's code
> > consistency let me know.
>
> Looking through the other struct definitions that are filled in from
> XML, we don't use any types that directly specify the number of bits.
> Instead we use int, long, and long long (sometimes with unsigned,
> sometimes without).
>
> On x86_64, long and long long are both 64 bits. I think I would just use
> "unsigned long" (assuming you agree to not use "-1" as a special value),
> then use %lu for the formatting string.
I'm actually switching to use uint32_t so %lu should work now, will
give it a try.
--
Cheers,
Alberto Ruiz
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] leasetime support for globally
2017-06-21 17:30 GMT+01:00 Laine Stump <la...@laine.org>: > On 06/21/2017 03:27 AM, Peter Krempa wrote: > > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote: > >> From: Alberto Ruiz <ar...@gnome.org> > > > > Missing commit message. > > And when composing the commit message, it's useful to include links to > the associated BZ. > > https://bugzilla.redhat.com/show_bug.cgi?id=913446 > > Also, I recall there being quite a lot of discussion in email (and > possibly IRC) about the fact that people *think* they want a > configurable lease time because they think that will eliminate cases of > a DHCP lease being lost while a domain is paused. It was pointed out > that lengthening the lease will *not* eliminate that problem (it just > makes it happen less often). > > As an alternate (and better) solution to the problem of lost leases, we > then added the "dhcp-authoritative" option to dnsmasq (commit > 4ac20b3ae4), which allows clients to re-acquire the same IP as they had > for an expired lease (as long as it hasn't been acquired by someone else > in the meantime, which is apparently unlikely unless all the other > addresses in the pool are already assigned). > > I'm not saying this to discourage the idea of making leasetime > configurable (I think we'd already agreed that it was reasonable to do > so, but there were two competing patches posted, and neither of them was > really push-ready), but just to make sure that nobody is disappointed if > the results don't lead to the behavior they're hoping for. > > > > > >> > >> --- > >> docs/schemas/basictypes.rng | 16 + > >> docs/schemas/network.rng | 8 +++ > >> src/conf/network_conf.c | 78 > ++- > >> src/conf/network_conf.h | 3 +- > >> src/network/bridge_driver.c | 49 +- > >> tests/networkxml2confdata/leasetime-days.conf | 17 + > >> tests/networkxml2confdata/leasetime-days.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-hours.conf| 17 + > >> tests/networkxml2confdata/leasetime-hours.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-infinite.conf | 17 + > >> tests/networkxml2confdata/leasetime-infinite.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-minutes.conf | 17 + > >> tests/networkxml2confdata/leasetime-minutes.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-seconds.conf | 17 + > >> tests/networkxml2confdata/leasetime-seconds.xml | 18 ++ > >> tests/networkxml2confdata/leasetime.conf | 17 + > >> tests/networkxml2confdata/leasetime.xml | 18 ++ > >> tests/networkxml2conftest.c | 7 ++ > >> 18 files changed, 368 insertions(+), 3 deletions(-) > >> create mode 100644 tests/networkxml2confdata/leasetime-days.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-days.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-hours.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-hours.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml > >> create mode 100644 tests/networkxml2confdata/leasetime.conf > >> create mode 100644 tests/networkxml2confdata/leasetime.xml > >> > >> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng > >> index 1b4f980e7..8a76c235a 100644 > >> --- a/docs/schemas/basictypes.rng > >> +++ b/docs/schemas/basictypes.rng > >> @@ -518,4 +518,20 @@ > >> > >> > >> > >> + > >> + > >> + seconds > >> + minutes > >> + hours > >> + days > >> + > >> + > > Maybe call this "timeUnit" in case some other attribute in the future > needs it? > > >> + > >> + > >> + > >> + -1 > >> + 4294967295 > >> + > >> + > >> + > >> > >> diff --git a/docs/schemas/ne
Re: [libvirt] [PATCH 1/3] leasetime support for globally
2017-06-21 17:30 GMT+01:00 Laine Stump <la...@laine.org>: > On 06/21/2017 03:27 AM, Peter Krempa wrote: > > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote: > >> From: Alberto Ruiz <ar...@gnome.org> > > > > Missing commit message. > > And when composing the commit message, it's useful to include links to > the associated BZ. > > https://bugzilla.redhat.com/show_bug.cgi?id=913446 > > Also, I recall there being quite a lot of discussion in email (and > possibly IRC) about the fact that people *think* they want a > configurable lease time because they think that will eliminate cases of > a DHCP lease being lost while a domain is paused. It was pointed out > that lengthening the lease will *not* eliminate that problem (it just > makes it happen less often). > > As an alternate (and better) solution to the problem of lost leases, we > then added the "dhcp-authoritative" option to dnsmasq (commit > 4ac20b3ae4), which allows clients to re-acquire the same IP as they had > for an expired lease (as long as it hasn't been acquired by someone else > in the meantime, which is apparently unlikely unless all the other > addresses in the pool are already assigned). > > I'm not saying this to discourage the idea of making leasetime > configurable (I think we'd already agreed that it was reasonable to do > so, but there were two competing patches posted, and neither of them was > really push-ready), but just to make sure that nobody is disappointed if > the results don't lead to the behavior they're hoping for. > > > > > >> > >> --- > >> docs/schemas/basictypes.rng | 16 + > >> docs/schemas/network.rng | 8 +++ > >> src/conf/network_conf.c | 78 > ++- > >> src/conf/network_conf.h | 3 +- > >> src/network/bridge_driver.c | 49 +- > >> tests/networkxml2confdata/leasetime-days.conf | 17 + > >> tests/networkxml2confdata/leasetime-days.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-hours.conf| 17 + > >> tests/networkxml2confdata/leasetime-hours.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-infinite.conf | 17 + > >> tests/networkxml2confdata/leasetime-infinite.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-minutes.conf | 17 + > >> tests/networkxml2confdata/leasetime-minutes.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-seconds.conf | 17 + > >> tests/networkxml2confdata/leasetime-seconds.xml | 18 ++ > >> tests/networkxml2confdata/leasetime.conf | 17 + > >> tests/networkxml2confdata/leasetime.xml | 18 ++ > >> tests/networkxml2conftest.c | 7 ++ > >> 18 files changed, 368 insertions(+), 3 deletions(-) > >> create mode 100644 tests/networkxml2confdata/leasetime-days.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-days.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-hours.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-hours.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml > >> create mode 100644 tests/networkxml2confdata/leasetime.conf > >> create mode 100644 tests/networkxml2confdata/leasetime.xml > >> > >> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng > >> index 1b4f980e7..8a76c235a 100644 > >> --- a/docs/schemas/basictypes.rng > >> +++ b/docs/schemas/basictypes.rng > >> @@ -518,4 +518,20 @@ > >> > >> > >> > >> + > >> + > >> + seconds > >> + minutes > >> + hours > >> + days > >> + > >> + > > Maybe call this "timeUnit" in case some other attribute in the future > needs it? > > >> + > >> + > >> + > >> + -1 > >> + 4294967295 > >> + > >> + > >> + > >> > >> diff --git a/docs/schemas/ne
Re: [libvirt] [PATCH 1/3] leasetime support for globally
Hello Laine, 2017-06-21 17:30 GMT+01:00 Laine Stump <la...@laine.org>: > On 06/21/2017 03:27 AM, Peter Krempa wrote: > > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote: > >> From: Alberto Ruiz <ar...@gnome.org> > > > > Missing commit message. > > And when composing the commit message, it's useful to include links to > the associated BZ. > > https://bugzilla.redhat.com/show_bug.cgi?id=913446 > > Also, I recall there being quite a lot of discussion in email (and > possibly IRC) about the fact that people *think* they want a > configurable lease time because they think that will eliminate cases of > a DHCP lease being lost while a domain is paused. It was pointed out > that lengthening the lease will *not* eliminate that problem (it just > makes it happen less often). > > As an alternate (and better) solution to the problem of lost leases, we > then added the "dhcp-authoritative" option to dnsmasq (commit > 4ac20b3ae4), which allows clients to re-acquire the same IP as they had > for an expired lease (as long as it hasn't been acquired by someone else > in the meantime, which is apparently unlikely unless all the other > addresses in the pool are already assigned). > > I'm not saying this to discourage the idea of making leasetime > configurable (I think we'd already agreed that it was reasonable to do > so, but there were two competing patches posted, and neither of them was > really push-ready), but just to make sure that nobody is disappointed if > the results don't lead to the behavior they're hoping for. > My main motivation _was_ the loss of IP addresses after reboot on GNOME Boxes. However, given that I've written the patches already and some people might find this useful, I'm okay with going ahead and get them ready for approval. > > > >> > >> --- > >> docs/schemas/basictypes.rng | 16 + > >> docs/schemas/network.rng | 8 +++ > >> src/conf/network_conf.c | 78 > ++- > >> src/conf/network_conf.h | 3 +- > >> src/network/bridge_driver.c | 49 +- > >> tests/networkxml2confdata/leasetime-days.conf | 17 + > >> tests/networkxml2confdata/leasetime-days.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-hours.conf| 17 + > >> tests/networkxml2confdata/leasetime-hours.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-infinite.conf | 17 + > >> tests/networkxml2confdata/leasetime-infinite.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-minutes.conf | 17 + > >> tests/networkxml2confdata/leasetime-minutes.xml | 18 ++ > >> tests/networkxml2confdata/leasetime-seconds.conf | 17 + > >> tests/networkxml2confdata/leasetime-seconds.xml | 18 ++ > >> tests/networkxml2confdata/leasetime.conf | 17 + > >> tests/networkxml2confdata/leasetime.xml | 18 ++ > >> tests/networkxml2conftest.c | 7 ++ > >> 18 files changed, 368 insertions(+), 3 deletions(-) > >> create mode 100644 tests/networkxml2confdata/leasetime-days.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-days.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-hours.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-hours.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml > >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf > >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml > >> create mode 100644 tests/networkxml2confdata/leasetime.conf > >> create mode 100644 tests/networkxml2confdata/leasetime.xml > >> > >> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng > >> index 1b4f980e7..8a76c235a 100644 > >> --- a/docs/schemas/basictypes.rng > >> +++ b/docs/schemas/basictypes.rng > >> @@ -518,4 +518,20 @@ > >> > >> > >> > >> + > >> + > >> + seconds > >> + minutes > >> + hours > >> + days > >> + > >> + > > Maybe call this "timeUnit" in case some other attribute in the fut
Re: [libvirt] [PATCH 1/3] leasetime support for globally
Hello Peter, Thanks for the comments, I will update the patch with your feedback, and sorry I thought I rebased it before I sent it, will fix that too. 2017-06-21 8:27 GMT+01:00 Peter Krempa <pkre...@redhat.com>: > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote: > > From: Alberto Ruiz <ar...@gnome.org> > > Missing commit message. > > > > > --- > > docs/schemas/basictypes.rng | 16 + > > docs/schemas/network.rng | 8 +++ > > src/conf/network_conf.c | 78 > ++- > > src/conf/network_conf.h | 3 +- > > src/network/bridge_driver.c | 49 +- > > tests/networkxml2confdata/leasetime-days.conf | 17 + > > tests/networkxml2confdata/leasetime-days.xml | 18 ++ > > tests/networkxml2confdata/leasetime-hours.conf| 17 + > > tests/networkxml2confdata/leasetime-hours.xml | 18 ++ > > tests/networkxml2confdata/leasetime-infinite.conf | 17 + > > tests/networkxml2confdata/leasetime-infinite.xml | 18 ++ > > tests/networkxml2confdata/leasetime-minutes.conf | 17 + > > tests/networkxml2confdata/leasetime-minutes.xml | 18 ++ > > tests/networkxml2confdata/leasetime-seconds.conf | 17 + > > tests/networkxml2confdata/leasetime-seconds.xml | 18 ++ > > tests/networkxml2confdata/leasetime.conf | 17 + > > tests/networkxml2confdata/leasetime.xml | 18 ++ > > tests/networkxml2conftest.c | 7 ++ > > 18 files changed, 368 insertions(+), 3 deletions(-) > > create mode 100644 tests/networkxml2confdata/leasetime-days.conf > > create mode 100644 tests/networkxml2confdata/leasetime-days.xml > > create mode 100644 tests/networkxml2confdata/leasetime-hours.conf > > create mode 100644 tests/networkxml2confdata/leasetime-hours.xml > > create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf > > create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml > > create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf > > create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml > > create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf > > create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml > > create mode 100644 tests/networkxml2confdata/leasetime.conf > > create mode 100644 tests/networkxml2confdata/leasetime.xml > > > > diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng > > index 1b4f980e7..8a76c235a 100644 > > --- a/docs/schemas/basictypes.rng > > +++ b/docs/schemas/basictypes.rng > > @@ -518,4 +518,20 @@ > > > > > > > > + > > + > > + seconds > > + minutes > > + hours > > + days > > + > > + > > + > > + > > + > > + -1 > > + 4294967295 > > + > > + > > + > > > > diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng > > index 1a18e64b2..4b8056ab6 100644 > > --- a/docs/schemas/network.rng > > +++ b/docs/schemas/network.rng > > @@ -340,6 +340,14 @@ > > > > > > + > > + > > + > > + name="leaseTimeUnit"/> > > This does not follow the XML style used everywhere else. > > > + > > + > > + > > + > > > > > > name="ipAddr"/> > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > > index aa397768c..6f051493f 100644 > > --- a/src/conf/network_conf.c > > +++ b/src/conf/network_conf.c > > @@ -30,6 +30,8 @@ > > #include > > #include > > #include > > +#include > > +#include > > > > #include "virerror.h" > > #include "datatypes.h" > > @@ -1031,12 +1033,82 @@ virNetworkDHCPHostDefParseXML(const char > *networkName, > > return ret; > > } > > > > +static int64_t > > +virNetworkDHCPDefGetLeaseTime (xmlNodePtr node, > > + xmlXPathContextPtr ctxt, > > + bool *error) > > We usually return error from the function and if necessary return the > value through pointer in arguments (backwards as you did).
Re: [libvirt] [PATCH] leasetime support per and
I'm attaching the patch because I've been unable to use send-email properly. 2) is a good point, I wanted to discuss it here. Yes 120 is the minimum allowed by dnsmasq, erroring out seems fine to me, I'll change the patch accordingly. On Thu, Oct 13, 2016 at 7:07 AM, Michal Privoznik <mpriv...@redhat.com> wrote: > On 13.10.2016 00:03, Alberto Ruiz wrote: > > Support for custom dhcp wide and per host leasetime. > > > > It is specified as a child tag for : > > > > 24h > > ... > > > > > > And as an attribute for : > > > > > > > > > > These are the different notations: > > > > -1 (infinite/unlimited lease) > > 120 (seconds are the default unit, 120 seconds is the minimum, if less > is > > specified it will use 120) > > 300s (seconds) > > 5m (minutes) > > 24h (hours) > > 7d (days) > > --- > > I know I'm stepping on a moving train (sorry for that), but I have two > points to raise: > > 1) use git send-email, this patch is mangled by your MTA and does not > apply. > 2) 120 seconds is minimum because of dnsmasq? If so, I think we should > error out instead of silently changing this to a different value behind > user's back. > > Michal > -- Alberto Ruiz Associate Engineering Manager - Desktop Management Tools Red Hat From d48f957ab78310d864b356b4a25b9a29722ca736 Mon Sep 17 00:00:00 2001 From: Alberto Ruiz <ar...@gnome.org> Date: Thu, 6 Oct 2016 21:29:40 +0100 Subject: [PATCH] leasetime support per and Support for custom dhcp wide and per host leasetime. It is specified as a child tag for : 24h ... And as an attribute for : These are the different notations: -1 (infinite/unlimited lease) 120 (seconds are the default unit, 120 seconds is the minimum, if less is specified it will use 120) 300s (seconds) 5m (minutes) 24h (hours) 7d (days) --- docs/schemas/basictypes.rng| 5 + docs/schemas/network.rng | 8 ++ src/conf/network_conf.c| 86 +- src/conf/network_conf.h| 4 +- src/libvirt_private.syms | 1 + src/network/bridge_driver.c| 132 + src/network/bridge_driver.h| 1 + src/util/virdnsmasq.c | 106 +++-- src/util/virdnsmasq.h | 2 + .../dhcp6-nat-network.hostsfile| 7 ++ tests/networkxml2confdata/dhcp6-network.hostsfile | 5 + .../dhcp6host-routed-network.hostsfile | 7 ++ tests/networkxml2confdata/leasetime-days.conf | 17 +++ tests/networkxml2confdata/leasetime-days.xml | 18 +++ tests/networkxml2confdata/leasetime-host.conf | 16 +++ tests/networkxml2confdata/leasetime-host.hostsfile | 6 + tests/networkxml2confdata/leasetime-host.xml | 22 tests/networkxml2confdata/leasetime-hours.conf | 17 +++ tests/networkxml2confdata/leasetime-hours.xml | 18 +++ tests/networkxml2confdata/leasetime-infinite.conf | 17 +++ tests/networkxml2confdata/leasetime-infinite.xml | 18 +++ tests/networkxml2confdata/leasetime-minutes.conf | 17 +++ tests/networkxml2confdata/leasetime-minutes.xml| 18 +++ tests/networkxml2confdata/leasetime-seconds.conf | 17 +++ tests/networkxml2confdata/leasetime-seconds.xml| 18 +++ tests/networkxml2confdata/leasetime.conf | 17 +++ tests/networkxml2confdata/leasetime.xml| 18 +++ .../nat-network-dns-srv-record-minimal.hostsfile | 2 + .../nat-network-dns-srv-record.hostsfile | 2 + .../nat-network-dns-txt-record.hostsfile | 2 + .../nat-network-name-with-quotes.hostsfile | 2 + tests/networkxml2confdata/nat-network.hostsfile| 2 + tests/networkxml2conftest.c| 47 ++-- 33 files changed, 597 insertions(+), 78 deletions(-) create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6host-routed-network.hostsfile create mode 100644 tests/networkxml2confdata/leasetime-days.conf create mode 100644 tests/networkxml2confdata/leasetime-days.xml create mode 100644 tests/networkxml2confdata/leasetime-host.conf create mode 100644 tests/networkxml2confdata/leasetime-host.hostsfile create mode 100644 tests/networkxml2confdata/leasetime-host.xml create mode 100644 tests/networkxml2confdata/leasetime-hours.conf create mode 100644 tests/networkxml2confdata/leasetime-hours.xml create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml create mode 100644 tests/networkxml2co
[libvirt] [PATCH] leasetime support per and
EE(actualconf);
+VIR_FREE(actualhosts);
VIR_FREE(pidfile);
virCommandFree(cmd);
virObjectUnref(obj);
@@ -70,20 +87,24 @@ testCompareXMLToConfHelper(const void *data)
int result = -1;
const testInfo *info = data;
char *inxml = NULL;
-char *outxml = NULL;
+char *outconf = NULL;
+char *outhostsfile = NULL;
if (virAsprintf(, "%s/networkxml2confdata/%s.xml",
abs_srcdir, info->name) < 0 ||
-virAsprintf(, "%s/networkxml2confdata/%s.conf",
+virAsprintf(, "%s/networkxml2confdata/%s.conf",
+abs_srcdir, info->name) < 0 ||
+virAsprintf(, "%s/networkxml2confdata/%s.hostsfile",
abs_srcdir, info->name) < 0) {
goto cleanup;
}
-result = testCompareXMLToConfFiles(inxml, outxml, info->caps);
+result = testCompareXMLToConfFiles(inxml, outconf, outhostsfile,
info->caps);
cleanup:
VIR_FREE(inxml);
-VIR_FREE(outxml);
+VIR_FREE(outconf);
+VIR_FREE(outhostsfile);
return result;
}
@@ -129,6 +150,14 @@ mymain(void)
DO_TEST("dhcp6-network", dhcpv6);
DO_TEST("dhcp6-nat-network", dhcpv6);
DO_TEST("dhcp6host-routed-network", dhcpv6);
+DO_TEST("leasetime", dhcpv6);
+DO_TEST("leasetime-seconds", dhcpv6);
+DO_TEST("leasetime-hours", dhcpv6);
+DO_TEST("leasetime-minutes", dhcpv6);
+DO_TEST("leasetime-hours", dhcpv6);
+DO_TEST("leasetime-days", dhcpv6);
+DO_TEST("leasetime-infinite", dhcpv6);
+DO_TEST("leasetime-host", dhcpv6);
virObjectUnref(dhcpv6);
virObjectUnref(full);
--
2.9.3
--
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
Re: [libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq
On Tue, Apr 19, 2016 at 2:34 AM, Laine Stump <la...@laine.org> 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 <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? . 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 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:
>
>
> 3600
>
>
>
>
>
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 that would override the leasetime directly under
> .
>
> (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(>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
Re: [libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq
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? . 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 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(>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
[libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq
Hello,
I've been asked to send this patch to the list for review which I
originally posted at rhbz#913446 [0].
My main motivation is to be able to set a saner default for dhcp leases in
Boxes for development machines where the instant volatility of the dhcp
lease makes it impossible to reliably work with services after the machines
have to be rebooted. One has to discover the ip again... setup ssh keys...
This patch is a first attempt so feel free to let me know if I'm aiming at
the right direction.
Thanks!
[0]https://bugzilla.redhat.com/show_bug.cgi?id=913446
--
Alberto Ruiz
Associate Engineering Manager - Desktop Management Tools
Red Hat
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
---
docs/schemas/basictypes.rng | 5 +
docs/schemas/network.rng| 3 +++
src/conf/network_conf.c | 8 +++-
src/network/bridge_driver.c | 10 --
src/util/virsocketaddr.h| 5 +++--
.../isolated-network-with-lease-time.conf | 17 +
.../isolated-network-with-lease-time.xml| 11 +++
tests/networkxml2conftest.c | 1 +
8 files changed, 55 insertions(+), 5 deletions(-)
create mode 100644 tests/networkxml2confdata/isolated-network-with-lease-time.conf
create mode 100644 tests/networkxml2confdata/isolated-network-with-lease-time.xml
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index a83063a..07aeba4 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -470,4 +470,9 @@
+
+
+ [1-9][0-9]*[hm]?|infinite
+
+
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 4edb6eb..6434c1b 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -330,6 +330,9 @@
+
+
+
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(>address));
}
-
static int
virSocketAddrRangeParseXML(const char *networkName,
virNetworkIpDefPtr ipdef,
@@ -890,6 +893,9 @@ virSocketAddrRangeParseXML(const char *networkName,
virNetworkIpDefPrefix(ipdef)) < 0)
goto cleanup;
+/* TODO: Sanity check the lease string */
+range->lease = virXMLPropString(node, "lease");
+
ret = 0;
cleanup:
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 73236ff..afb79f4 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1187,8 +1187,14 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
!(eaddr = virSocketAddrFormat(>ranges[r].end)))
goto cleanup;
-virBufferAsprintf(, "dhcp-range=%s,%s\n",
- saddr, eaddr);
+if (ipdef->ranges[r].lease) {
+virBufferAsprintf(, "dhcp-range=%s,%s,%s\n",
+ saddr, eaddr, ipdef->ranges[r].lease);
+} else {
+ virBufferAsprintf(, "dhcp-range=%s,%s\n",
+ saddr, eaddr);
+}
+
VIR_FREE(saddr);
VIR_FREE(eaddr);
thisRange = virSocketAddrGetRange(>ranges[r].start,
diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h
index c7aaa61..4baeda5 100644
--- a/src/util/virsocketaddr.h
+++ b/src/util/virsocketaddr.h
@@ -63,8 +63,9 @@ typedef virSocketAddr *virSocketAddrPtr;
typedef struct _virSocketAddrRange virSocketAddrRange;
typedef virSocketAddrRange *virSocketAddrRangePtr;
struct _virSocketAddrRange {
-virSocketAddr start;
-virSocketAddr end;
+virSocketAddr start;
+virSocketAddr end;
+char *lease;
};
typedef struct _virPortRange virPortRange;
diff --git a/tests/networkxml2confdata/isolated-network-with-lease-time.conf b/tests/networkxml2confdata/isolated-network-with-lease-time.conf
new file mode 100644
index 000..a02dad
