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@xxxxxxxxx wrote: > From: Alberto Ruiz <aruiz@xxxxxxxxx> > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
