12:45 a.m.
Peter Krempa (2):
virsh: Refactor parseRateStr to avoid false-positive uninitialized
variable
virsh: Properly reject 'floor' settings in cmdAttachInterface
tools/virsh-domain.c | 96 ++++++++++++++++++++++------------------------------
1 file changed, 41 insertions(+), 55 deletions(-)
--
2.4.5
12:45 a.m.
New subject: [libvirt] [PATCH 1/2] virsh: Refactor parseRateStr to avoid false-positive uninitialized variable
Commit 6983d6d2 tried to improve parseRateStr but broke the build
instead for compilers that were not able to properly introspect the for
loop indexed by the enum resulting into the following error:
virsh-domain.c: In function 'parseRateStr':
virsh-domain.c:916:13: error: 'field_name' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
vshError(ctl, _("malformed %s field"), field_name);
^
virsh-domain.c:915:13: error: 'tmp' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
if (virStrToLong_ullp(token, NULL, 10, tmp) < 0) {
^
Rather than trying to fix the code, refactor the function again by
reusing virStringSplit.
---
tools/virsh-domain.c | 88 +++++++++++++++++++++-------------------------------
1 file changed, 35 insertions(+), 53 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a957836..9f54691 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -863,67 +863,49 @@ static const vshCmdOptDef opts_attach_interface[] = {
};
/* parse inbound and outbound which are in the format of
- * 'average,peak,burst', in which peak and burst are optional,
+ * 'average,peak,burst,floor', in which peak and burst are optional,
* thus 'average,,burst' and 'average,peak' are also legal. */
-static int parseRateStr(vshControl *ctl,
- const char *rateStr,
- virNetDevBandwidthRatePtr rate)
-{
- char *token;
- char *next;
- char *saveptr = NULL;
- enum {
- AVERAGE, PEAK, BURST, FLOOR
- } state;
- int ret = -1;
-
- if (!rateStr)
- return -1;
-
- next = vshStrdup(ctl, rateStr);
-
- for (state = AVERAGE; state <= FLOOR; state++) {
- unsigned long long *tmp;
- const char *field_name;
-
- if (!(token = strtok_r(next, ",", &saveptr)))
- break;
- next = NULL;
-
- switch (state) {
- case AVERAGE:
- tmp = &rate->average;
- field_name = "average";
- break;
- case PEAK:
- tmp = &rate->peak;
- field_name = "peak";
- break;
+#define VSH_PARSE_RATE_FIELD(index, name) \
+ do { \
+ if (index < ntok &&
\
+ *tok[index] != '\0' &&
\
+ virStrToLong_ullp(tok[index], NULL, 10, &rate->name) < 0) {
\
+ vshError(ctl, _("field '%s' is malformed"), #name);
\
+ goto cleanup; \
+ } \
+ } while (0)
- case BURST:
- tmp = &rate->burst;
- field_name = "burst";
- break;
+static int
+vshParseRateStr(vshControl *ctl,
+ const char *rateStr,
+ virNetDevBandwidthRatePtr rate)
+{
+ char **tok = NULL;
+ size_t ntok;
+ int ret = -1;
- case FLOOR:
- tmp = &rate->floor;
- field_name = "floor";
- break;
- }
+ if (!(tok = virStringSplitCount(rateStr, ",", 0, &ntok)))
+ return -1;
- if (virStrToLong_ullp(token, NULL, 10, tmp) < 0) {
- vshError(ctl, _("malformed %s field"), field_name);
- goto cleanup;
- }
+ if (ntok > 4) {
+ vshError(ctl, _("Rate string '%s' has too many fields"),
rateStr);
+ goto cleanup;
}
+ VSH_PARSE_RATE_FIELD(0, average);
+ VSH_PARSE_RATE_FIELD(1, peak);
+ VSH_PARSE_RATE_FIELD(2, burst);
+ VSH_PARSE_RATE_FIELD(3, floor);
+
ret = 0;
cleanup:
- VIR_FREE(next);
+ virStringFreeList(tok);
return ret;
}
+#undef VSH_PARSE_RATE_FIELD
+
static bool
cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
{
@@ -979,7 +961,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
if (inboundStr) {
memset(&inbound, 0, sizeof(inbound));
- if (parseRateStr(ctl, inboundStr, &inbound) < 0)
+ if (vshParseRateStr(ctl, inboundStr, &inbound) < 0)
goto cleanup;
if (!inbound.average && !inbound.floor) {
vshError(ctl, _("either inbound average or floor is mandatory"));
@@ -988,7 +970,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
}
if (outboundStr) {
memset(&outbound, 0, sizeof(outbound));
- if (parseRateStr(ctl, outboundStr, &outbound) < 0)
+ if (vshParseRateStr(ctl, outboundStr, &outbound) < 0)
goto cleanup;
if (outbound.average == 0) {
vshError(ctl, _("outbound average is mandatory"));
@@ -3307,7 +3289,7 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd)
memset(&outbound, 0, sizeof(outbound));
if (inboundStr) {
- if (parseRateStr(ctl, inboundStr, &inbound) < 0)
+ if (vshParseRateStr(ctl, inboundStr, &inbound) < 0)
goto cleanup;
/* we parse the rate as unsigned long long, but the API
* only accepts UINT */
@@ -3349,7 +3331,7 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd)
}
if (outboundStr) {
- if (parseRateStr(ctl, outboundStr, &outbound) < 0)
+ if (vshParseRateStr(ctl, outboundStr, &outbound) < 0)
goto cleanup;
if (outbound.average > UINT_MAX || outbound.peak > UINT_MAX ||
outbound.burst > UINT_MAX) {
--
2.4.5
3:21 a.m.
New subject: [libvirt] [PATCH 1/2] virsh: Refactor parseRateStr to avoid false-positive uninitialized variable
On 12.08.2015 07:45, Peter Krempa wrote:
Commit 6983d6d2 tried to improve parseRateStr but broke the build
instead for compilers that were not able to properly introspect the for
loop indexed by the enum resulting into the following error:
virsh-domain.c: In function 'parseRateStr':
virsh-domain.c:916:13: error: 'field_name' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
vshError(ctl, _("malformed %s field"), field_name);
^
virsh-domain.c:915:13: error: 'tmp' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
if (virStrToLong_ullp(token, NULL, 10, tmp) < 0) {
^
Rather than trying to fix the code, refactor the function again by
reusing virStringSplit.
---
tools/virsh-domain.c | 88 +++++++++++++++++++++-------------------------------
1 file changed, 35 insertions(+), 53 deletions(-)
ACK
Michal
3:54 a.m.
New subject: [libvirt] [PATCH 1/2] virsh: Refactor parseRateStr to avoid false-positive uninitialized variable
On Wed, Aug 12, 2015 at 10:21:39 +0200, Michal Privoznik wrote:
On 12.08.2015 07:45, Peter Krempa wrote:
> Commit 6983d6d2 tried to improve parseRateStr but broke the build
> instead for compilers that were not able to properly introspect the for
> loop indexed by the enum resulting into the following error:
>
> virsh-domain.c: In function 'parseRateStr':
> virsh-domain.c:916:13: error: 'field_name' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
> vshError(ctl, _("malformed %s field"), field_name);
> ^
> virsh-domain.c:915:13: error: 'tmp' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
> if (virStrToLong_ullp(token, NULL, 10, tmp) < 0) {
> ^
> Rather than trying to fix the code, refactor the function again by
> reusing virStringSplit.
> ---
> tools/virsh-domain.c | 88 +++++++++++++++++++++-------------------------------
> 1 file changed, 35 insertions(+), 53 deletions(-)
>
ACK
I've pushed this one. Thanks
Michal
12:45 a.m.
New subject: [libvirt] [PATCH 2/2] virsh: Properly reject 'floor' settings in cmdAttachInterface
cmdAttachInterface doesn't support the 'floor' field that was added in
d7f5c88961b52 but that commit didn't properly reject it from
cmdAttachInterface where it's unused.
---
tools/virsh-domain.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9f54691..94e7a5c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -963,8 +963,12 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
memset(&inbound, 0, sizeof(inbound));
if (vshParseRateStr(ctl, inboundStr, &inbound) < 0)
goto cleanup;
- if (!inbound.average && !inbound.floor) {
- vshError(ctl, _("either inbound average or floor is mandatory"));
+ if (!inbound.average) {
+ vshError(ctl, _("inbound average is mandatory"));
+ goto cleanup;
+ }
+ if (inbound.floor) {
+ vshError(ctl, _("inbound floor is unsupported yet"));
goto cleanup;
}
}
--
2.4.5
2:59 a.m.
New subject: [libvirt] [PATCH 2/2] virsh: Properly reject 'floor' settings in cmdAttachInterface
On 12.08.2015 07:45, Peter Krempa wrote:
cmdAttachInterface doesn't support the 'floor' field that
was added in
d7f5c88961b52 but that commit didn't properly reject it from
cmdAttachInterface where it's unused.
---
tools/virsh-domain.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9f54691..94e7a5c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -963,8 +963,12 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
memset(&inbound, 0, sizeof(inbound));
if (vshParseRateStr(ctl, inboundStr, &inbound) < 0)
goto cleanup;
- if (!inbound.average && !inbound.floor) {
- vshError(ctl, _("either inbound average or floor is mandatory"));
+ if (!inbound.average) {
+ vshError(ctl, _("inbound average is mandatory"));
+ goto cleanup;
+ }
+ if (inbound.floor) {
+ vshError(ctl, _("inbound floor is unsupported yet"));
goto cleanup;
}
}
Ah, it isn't. But it's not limitation of our API (which does support @floor of
course). It's a virsh limitation. So how about instead of you patch have the following
in?
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a957836..7cd521e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1041,12 +1041,16 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
if (inboundStr || outboundStr) {
virBufferAddLit(&buf, "<bandwidth>\n");
virBufferAdjustIndent(&buf, 2);
- if (inboundStr && inbound.average > 0) {
- virBufferAsprintf(&buf, "<inbound average='%llu'",
inbound.average);
+ if (inboundStr && (inbound.average || inbound.floor)) {
+ virBufferAddLit(&buf, "<inbound");
+ if (inbound.average > 0)
+ virBufferAsprintf(&buf, " average='%llu'",
inbound.average);
if (inbound.peak > 0)
virBufferAsprintf(&buf, " peak='%llu'",
inbound.peak);
if (inbound.burst > 0)
virBufferAsprintf(&buf, " burst='%llu'",
inbound.burst);
+ if (inbound.floor > 0)
+ virBufferAsprintf(&buf, " floor='%llu'",
inbound.floor);
virBufferAddLit(&buf, "/>\n");
}
if (outboundStr && outbound.average > 0) {
Michal
3:14 a.m.
New subject: [libvirt] [PATCH 2/2] virsh: Properly reject 'floor' settings in cmdAttachInterface
On Wed, Aug 12, 2015 at 09:59:08 +0200, Michal Privoznik wrote:
On 12.08.2015 07:45, Peter Krempa wrote:
> cmdAttachInterface doesn't support the 'floor' field that was added in
> d7f5c88961b52 but that commit didn't properly reject it from
> cmdAttachInterface where it's unused.
> ---
> tools/virsh-domain.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 9f54691..94e7a5c 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -963,8 +963,12 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> memset(&inbound, 0, sizeof(inbound));
> if (vshParseRateStr(ctl, inboundStr, &inbound) < 0)
> goto cleanup;
> - if (!inbound.average && !inbound.floor) {
> - vshError(ctl, _("either inbound average or floor is
mandatory"));
> + if (!inbound.average) {
> + vshError(ctl, _("inbound average is mandatory"));
> + goto cleanup;
> + }
> + if (inbound.floor) {
> + vshError(ctl, _("inbound floor is unsupported yet"));
> goto cleanup;
> }
> }
>
Ah, it isn't. But it's not limitation of our API (which does support @floor of
course). It's a virsh limitation. So how about instead of you patch have the following
in?
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a957836..7cd521e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1041,12 +1041,16 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
if (inboundStr || outboundStr) {
virBufferAddLit(&buf, "<bandwidth>\n");
virBufferAdjustIndent(&buf, 2);
- if (inboundStr && inbound.average > 0) {
- virBufferAsprintf(&buf, "<inbound average='%llu'",
inbound.average);
+ if (inboundStr && (inbound.average || inbound.floor)) {
+ virBufferAddLit(&buf, "<inbound");
+ if (inbound.average > 0)
+ virBufferAsprintf(&buf, " average='%llu'",
inbound.average);
if (inbound.peak > 0)
virBufferAsprintf(&buf, " peak='%llu'",
inbound.peak);
if (inbound.burst > 0)
virBufferAsprintf(&buf, " burst='%llu'",
inbound.burst);
+ if (inbound.floor > 0)
+ virBufferAsprintf(&buf, " floor='%llu'",
inbound.floor);
virBufferAddLit(&buf, "/>\n");
}
if (outboundStr && outbound.average > 0) {
Feel free to do it that way. I've noticed that the check didn't make
much sense as it was now and I didn't care enough to see whether it was
possible to use that.
Peter
3355
days inactive
3355
days old
6 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa