[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 gnome org wrote:
> From: Alberto Ruiz <aruiz 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]