Re: [PATCH v3 1/2] conf: Add option for 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:
>
>
>
>
>
>
>
>
>
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
>
> Signed-off-by: Julio Faracco
> ---
[...]
> 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;
> +}
> +
> +
[...]
Re: [PATCH v3 1/2] conf: Add option for settings
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:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
Signed-off-by: Julio Faracco
---
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 @@
+
+
+ seconds
+ mins
+ hours
Since seconds and hours are specified fully, I think "mins" should be
too. It's not any longer than "seconds" anyway :-)
+
+
+
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 @@
+
+
+
+
+
+
+
+
+
+
@@ -388,6 +398,16 @@
+
+
+
+
+
+
+
+
+
+
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 enu
[PATCH v3 1/2] conf: Add option for settings
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:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
Signed-off-by: Julio Faracco
---
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 @@
+
+
+ seconds
+ mins
+ hours
+
+
+
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 @@
+
+
+
+
+
+
+
+
+
+
@@ -388,6 +398,16 @@
+
+
+
+
+
+
+
+
+
+
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);
+
+/* 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_CON
