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

[Michal Privoznik]

On 4/22/20 10: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>
> ---
>   docs/schemas/basictypes.rng |   8 ++
>   docs/schemas/network.rng    |  20 +++++
>   src/conf/network_conf.c     | 159 +++++++++++++++++++++++++++++++-----
>   src/conf/network_conf.h     |  27 +++++-
>   src/libvirt_private.syms    |   3 +
>   src/network/bridge_driver.c |  56 +++++++++++--
>   src/network/bridge_driver.h |   1 +
>   src/test/test_driver.c      |   2 +-
>   src/util/virdnsmasq.c       |  60 +++++++++-----
>   src/util/virdnsmasq.h       |   3 +
>   src/vbox/vbox_network.c     |  16 ++--
>   tests/networkxml2conftest.c |  15 ++--
>   12 files changed, 306 insertions(+), 64 deletions(-)
> 
> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> index 81465273c8..271ed18afb 100644
> --- a/docs/schemas/basictypes.rng
> +++ b/docs/schemas/basictypes.rng
> @@ -607,4 +607,12 @@
>       </element>
>     </define>
>   
> +  <define name="leaseUnit">
> +    <choice>
> +      <value>seconds</value>
> +      <value>mins</value>
> +      <value>hours</value>

Since seconds and hours are specified fully, I think "mins" should be 
too. It's not any longer than "seconds" anyway :-)

> +    </choice>
> +  </define>
> +
>   </grammar>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 60453225d6..88b6f4dfdd 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -371,6 +371,16 @@
>                         <element name="range">
>                           <attribute name="start"><ref name="ipAddr"/></attribute>
>                           <attribute name="end"><ref name="ipAddr"/></attribute>
> +                        <interleave>
> +                          <optional>
> +                            <element name="lease">
> +                              <attribute name="expiry"><ref name="unsignedLong"/></attribute>
> +                              <optional>
> +                                <attribute name="unit"><ref name="leaseUnit"/></attribute>
> +                              </optional>
> +                            </element>
> +                          </optional>
> +                        </interleave>
>                         </element>
>                       </zeroOrMore>
>                       <zeroOrMore>
> @@ -388,6 +398,16 @@
>                             <attribute name="name"><text/></attribute>
>                           </choice>
>                           <attribute name="ip"><ref name="ipAddr"/></attribute>
> +                        <interleave>
> +                          <optional>
> +                            <element name="lease">
> +                              <attribute name="expiry"><ref name="unsignedLong"/></attribute>
> +                              <optional>
> +                                <attribute name="unit"><ref name="leaseUnit"/></attribute>
> +                              </optional>
> +                            </element>
> +                          </optional>
> +                        </interleave>
>                         </element>
>                       </zeroOrMore>
>                       <optional>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 819b645df7..286a0edb7c 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -70,6 +70,13 @@ VIR_ENUM_IMPL(virNetworkTaint,
>                 "hook-script",
>   );
>   
> +VIR_ENUM_IMPL(virNetworkDHCPLeaseTimeUnit,
> +              VIR_NETWORK_DHCP_LEASETIME_UNIT_LAST,
> +              "seconds",
> +              "mins",
> +              "hours",
> +);
> +
>   static virClassPtr virNetworkXMLOptionClass;
>   
>   static void
> @@ -132,12 +139,20 @@ virNetworkForwardPfDefClear(virNetworkForwardPfDefPtr def)
>   }
>   
>   
> +static void
> +virNetworkDHCPLeaseTimeDefClear(virNetworkDHCPLeaseTimeDefPtr lease)
> +{
> +    VIR_FREE(lease);
> +}
> +
> +
>   static void
>   virNetworkDHCPHostDefClear(virNetworkDHCPHostDefPtr def)
>   {
>       VIR_FREE(def->mac);
>       VIR_FREE(def->id);
>       VIR_FREE(def->name);
> +    VIR_FREE(def->lease);
>   }
>   
>   
> @@ -145,6 +160,9 @@ static void
>   virNetworkIPDefClear(virNetworkIPDefPtr def)
>   {
>       VIR_FREE(def->family);
> +
> +    while (def->nranges)
> +        virNetworkDHCPLeaseTimeDefClear(def->ranges[--def->nranges].lease);
>       VIR_FREE(def->ranges);
>   
>       while (def->nhosts)
> @@ -391,11 +409,55 @@ int virNetworkIPDefNetmask(const virNetworkIPDef *def,
>   
>   
>   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);

Ouch. This is unsafe. Firstly, if an uses specifies say 
unit="microfornight" TypeFromString() will return -1 because the string 
is not from the enum. Then, assigning -1 to the 
virNetworkDHCPLeaseTimeUnitType enum which doesn't contain that value is 
dangerous too...

> +
> +    /* 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;
> +        }
> +    }
> +
> +    *lease = new_lease;
> +
> +    return 0;
> +}
> +
> +


> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index f06099297a..87f0452611 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -966,6 +966,30 @@ static int networkConnectIsAlive(virConnectPtr conn G_GNUC_UNUSED)
>   }
>   
>   
> +static char *
> +networkBuildDnsmasqLeaseTime(virNetworkDHCPLeaseTimeDefPtr lease)
> +{
> +    char *leasetime = NULL;
> +    const char *unit;
> +    g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (!lease)
> +        return NULL;
> +
> +    if (lease->expiry == 0) {
> +        virBufferAddLit(&buf, "infinite");
> +    } else {
> +        unit = virNetworkDHCPLeaseTimeUnitTypeToString(lease->unit);
> +        /* We get only first compatible char from string: 's', 'm' or 'h' */
> +        virBufferAsprintf(&buf, "%lu%c", lease->expiry, unit[0]);

... because here unit will be NULL and dereferenced.

Michal