On 4/22/20 4:05 PM, Julio Faracco wrote: > If an user is trying to configure a dhcp neetwork settings, it is not > possible to change the leasetime of a range or a host entry. This is > available using dnsmasq extra options, but they are associated with > dhcp-range or dhcp-hosts fields. This patch implements a leasetime for > range and hosts tags. They can be defined under that settings: > > <dhcp> > <range ...> > <lease/> > </range> > <host ...> > <lease/> > </host> > </dhcp> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446 > > Signed-off-by: Julio Faracco <jcfaracco@xxxxxxxxx> > --- [...] > static int > -virSocketAddrRangeParseXML(const char *networkName, > - virNetworkIPDefPtr ipdef, > - xmlNodePtr node, > - virSocketAddrRangePtr range) > +virNetworkDHCPLeaseTimeDefParseXML(virNetworkDHCPLeaseTimeDefPtr *lease, > + xmlNodePtr node) > +{ > + virNetworkDHCPLeaseTimeDefPtr new_lease = *lease; > + g_autofree char *expiry = NULL, *unit = NULL; > + > + if (!(expiry = virXMLPropString(node, "expiry"))) > + return 0; > + > + if (VIR_ALLOC(new_lease) < 0) > + return -1; > + > + if (virStrToLong_ul(expiry, NULL, 10, &new_lease->expiry) < 0) > + return -1; > + > + if (!(unit = virXMLPropString(node, "unit"))) > + new_lease->unit = VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES; > + else > + new_lease->unit = virNetworkDHCPLeaseTimeUnitTypeFromString(unit); > + > + /* infinite */ > + if (new_lease->expiry > 0) { > + /* This boundary check is related to dnsmasq man page settings: > + * "The minimum lease time is two minutes." */ > + if ((new_lease->unit == VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS && > + new_lease->expiry < 120) || > + (new_lease->unit == VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES && > + new_lease->expiry < 2)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("The minimum lease time should be greater " > + "than 2 minutes")); > + return -1; Coverity pointed out there's a @new_lease leak here. In just a quick scan of the code it's not the only place and I wonder about the initialization of new_lease = *lease and then just VIR_ALLOC right over that... Probably should be initialized to NULL. Anyway - perhaps consider changing @expiry to @expirystr and @unit to @unitstr, then filling/using @expiry & @unit (instead of unitInt) for comparisons before the VIR_ALLOC(new_lease) and filling in the two fields once all the checks are done - probably allows those boundary checks to not span 4 lines... JMO... John > + } > + } > + > + *lease = new_lease; > + > + return 0; > +} > + > + [...]
