Thank you John for your review, however, before I send another patch I'd
like to know your (or someone else's) opinion on my notes/thoughts I
posted below.
Erik
On 09/08/2014 10:16 PM, John Ferlan wrote:
On 09/08/2014 10:49 AM, Erik Skultety wrote:
When spanning tree protocol is allowed in bridge settings, forward delay
value is set as well (default is 0 if omitted). Until now, there was no
check for delay value validity. Delay makes sense only as a positive
numerical value.
Note: However, even if you provide positive numerical value, brctl
utility only uses values from range <2,30>, so the number provided can
be modified (kernel most likely) to fall within this range.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1125764
---
docs/schemas/network.rng | 2 +-
src/conf/network_conf.c | 18 +++++++++++++-----
src/util/virxml.c | 2 +-
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 0e7da89..ab41814 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -62,7 +62,7 @@
<optional>
<attribute name="delay">
- <data type="integer"/>
+ <data type="unsignedLong"/>
Changing the schema - not sure this is supposed to be changed since
we've already released as 'integer'.
Although unsignedLong is what the 'delay' is defined as in
network_conf.h - so technically it's correct.
Well, the only reason why I wanted to change it in the schema is that
there is an idea to do the schema check not only as a part of unit
testing, but to do it after XML domain editing (future feature) as well.
In that case, I think it might be correct, if we would stay consistent
with our definitions.
</attribute>
</optional>
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 9571ee1..4d6db8c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2016,6 +2016,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
xmlNodePtr save = ctxt->node;
xmlNodePtr bandwidthNode = NULL;
xmlNodePtr vlanNode;
+ int ret = 0;
if (VIR_ALLOC(def) < 0)
return NULL;
@@ -2078,8 +2079,15 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
stp = virXPathString("string(./bridge[1]/@stp)", ctxt);
def->stp = (stp && STREQ(stp, "off")) ? false : true;
- if (virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay) < 0)
- def->delay = 0;
+ ret = virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay);
+ if (ret == -2) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid delay value in network '%s'"),
+ def->name);
+ goto error;
+ } else if (ret < 0) {
+ def->delay = 0;
+ }
See [1] below...
You probably want to make a virStrToLong_ulp() directly here -
tmp = virXPathString("string(./bridge[1]/@delay)", ctxt);
if (tmp) {
if (virStrToLong_ulp(tmp, NULL, 10, &def->delay)) {
virReportError(VIR_ERR_XML_ERROR,
_("Invalid delay value in network '%s'"),
def->name);
VIR_FREE(tmp);
goto error;
}
VIR_FREE(tmp);
}
Yep, much better idea, although I went through possible callers of
virXPathULong, I think most of them (but not all) do not honor the
feature -1 being the max/all/everything. Now the problem is that we do
not check for negative values, we leave them as they are. If we fixed it
at one specific place, then we should stay consistent and do the check
on all other places. What's more, even if we did it (not sure if worth
the effort), we do not check for reasonable ranges, so in the end,
whether we checked for negative values or not, we'd end up with huge
values to be set which get ignored by qemu or kernel.
Although the extra VIR_FREE(tmp) in the error path could be eliminated
from this and other paths by setting tmp = NULL at the start and adding
one in the 'error:' label.
Good tip, I found a couple of lines where we actually forgot to free tmp
in case of an error, thanks.
tmp = virXPathString("string(./mac[1]/@address)", ctxt);
if (tmp) {
@@ -2126,7 +2134,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
goto error;
/* parse each portgroup */
for (i = 0; i < nPortGroups; i++) {
- int ret = virNetworkPortGroupParseXML(&def->portGroups[i],
+ ret = virNetworkPortGroupParseXML(&def->portGroups[i],
portGroupNodes[i], ctxt);
if (ret < 0)
goto error;
@@ -2147,7 +2155,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
goto error;
/* parse each addr */
for (i = 0; i < nIps; i++) {
- int ret = virNetworkIPDefParseXML(def->name, ipNodes[i],
+ ret = virNetworkIPDefParseXML(def->name, ipNodes[i],
ctxt, &def->ips[i]);
if (ret < 0)
goto error;
@@ -2168,7 +2176,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
goto error;
/* parse each definition */
for (i = 0; i < nRoutes; i++) {
- int ret = virNetworkRouteDefParseXML(def->name, routeNodes[i],
+ ret = virNetworkRouteDefParseXML(def->name, routeNodes[i],
ctxt, &def->routes[i]);
These each could just be :
if (func(args) < 0)
goto error;
if you were going to change them anyway (since ret isn't used)
Sure, you're right.
if (ret < 0)
goto error;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index cc4a85c..f730f5e 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -286,7 +286,7 @@ virXPathULongBase(const char *xpath,
ctxt->node = relnode;
if ((obj != NULL) && (obj->type == XPATH_STRING) &&
(obj->stringval != NULL) && (obj->stringval[0] != 0)) {
- if (virStrToLong_ul((char *) obj->stringval, NULL, base, value) < 0)
+ if (virStrToLong_ulp((char *) obj->stringval, NULL, base, value) < 0)
[1]
I believe this will be far too generic as there are a set of commands
that use -1 as a "feature" to indicate everything or all the time or in
some manner the max value.
There's quite a few callers of the callers to virXPathULongBase() that
would need to be checked to see if they "desire" allowing a negative value
There's some "related" commitid's you may want to look at '37e663ad,
'0e2d7305', 'c6212539'
John
ret = -2;
} else if ((obj != NULL) && (obj->type == XPATH_NUMBER) &&
(!(isnan(obj->floatval)))) {
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list