Re: [PATCH v3 1/2] conf: Add <lease/> option for <dhcp/> settings
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(a)gmail.com>
---
[...]
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;
+}
+
+
[...]