Re: [libvirt] [PATCH] leasetime support for <dhcp>
Sorry this keeps falling off the radar...
When posting a v2/v3/vX, please use [PATCH v2] prefix, and make each new
posting a top level patch
On 06/22/2017 08:44 PM, aruiz(a)gnome.org wrote:
From: Alberto Ruiz <aruiz(a)gnome.org>
Fixes #913446
Full bug link makes things slightly easier if the reviewer wants to look
In the commit message [lease at least give an example of the added XML values,
possibly what dnsmasq value it maps to. Makes grepping commit logs easier
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
These types of bits should go after the --- break below: they don't need to be
in the commit message but they are useful for reviewers
There's lots of style issues: please run 'make syntax-check' and fix the
warnings. Peter pointed out some of them already against your 6/21 posting but
they weren't addressed in this version:
https://www.redhat.com/archives/libvir-list/2017-June/msg00838.html
Drop the strcmp usage, we have a STREQ macro we use ('make syntax-check' might
warn but I'm not positive)
CC me on the v3 and I'll do a full review
Thanks,
Cole