[PATCH v3 1/2] conf: Add <lease/> option for <dhcp/> settings

[John Ferlan]

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 at 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;
> +}
> +
> +

[...]