[libvirt] [PATCH] conf: Use UNIT_MAX for checking max value of unsigned int
In the current scenario the controller parsing fails when no index is specified .As the docs point out that specifying index is optional, libvirt is expected to fill the index without erroring out. Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- Before the Patch: cat controller.xml <controller model="virtio-scsi" type="scsi" /> virsh attach-device vm1 controller.xml --persistent error: Failed to attach device from controller.xml error: internal error: Cannot parse controller index -1 After the patch: virsh attach-device vm1 controller.xml --persistent Device attached successfully --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..c52e67f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8374,7 +8374,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (idx) { unsigned int idxVal; if (virStrToLong_ui(idx, NULL, 10, &idxVal) < 0 || - idxVal > INT_MAX) { + idxVal > UINT_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse controller index %s"), idx); goto error; -- 2.1.0
On Monday, 14 November 2016 22:43:55 CET Nitesh Konkar wrote:
In the current scenario the controller parsing fails when no index is specified .As the docs point out that specifying index is optional, libvirt is expected to fill the index without erroring out.
Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> ---
There's something I don't understand -- it looks to me there's a mismatch between the test case you provide (and its output), and what the code part for it actually does.
Before the Patch: cat controller.xml <controller model="virtio-scsi" type="scsi" />
Your XML has no index="" attribute.
virsh attach-device vm1 controller.xml --persistent error: Failed to attach device from controller.xml error: internal error: Cannot parse controller index -1
After the patch: virsh attach-device vm1 controller.xml --persistent Device attached successfully --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..c52e67f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8374,7 +8374,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (idx) {
Here "idx" is non-null only if the attribute is actually found, which does not seem the case in the XML snippet above.
unsigned int idxVal; if (virStrToLong_ui(idx, NULL, 10, &idxVal) < 0 || - idxVal > INT_MAX) { + idxVal > UINT_MAX) {
This looks wrong to me: UINT_MAX is the maximum value that is going to fit into a 'unsigned int', so 'idxVal' will never be (even on error) greater than that. If the index must be non-negative, it looks like using virStrToLong_uip instead of virStrToLong_ui could work fine in rejecting negative values outright.
virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse controller index %s"), idx);
Considering this, I think the XML you used in your testsmost probably has index="-1" as attribute -- can you please confirm? Thanks, -- Pino Toscano
On Mon, Nov 14, 2016 at 11:21 PM, Pino Toscano <ptoscano@redhat.com> wrote:
On Monday, 14 November 2016 22:43:55 CET Nitesh Konkar wrote:
In the current scenario the controller parsing fails when no index is specified .As the docs point out that specifying index is optional, libvirt is expected to fill the index without erroring out.
Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> ---
There's something I don't understand -- it looks to me there's a mismatch between the test case you provide (and its output), and what the code part for it actually does.
Before the Patch: cat controller.xml <controller model="virtio-scsi" type="scsi" />
Your XML has no index="" attribute.
As documented here https://libvirt.org/formatdomain.html#elementsControllers , Since 1.3.5 the index is optional; if not specified, it will be auto-assigned to be the lowest unused index for the given controller type.
virsh attach-device vm1 controller.xml --persistent error: Failed to attach device from controller.xml error: internal error: Cannot parse controller index -1
virsh attach-device vm1 controller.xml --live and virsh attach-device vm1 controller.xml --config work fine.
After the patch: virsh attach-device vm1 controller.xml --persistent Device attached successfully --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..c52e67f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8374,7 +8374,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (idx) {
Here "idx" is non-null only if the attribute is actually found, which does not seem the case in the XML snippet above.
unsigned int idxVal; if (virStrToLong_ui(idx, NULL, 10, &idxVal) < 0 || - idxVal > INT_MAX) { + idxVal > UINT_MAX) {
This looks wrong to me: UINT_MAX is the maximum value that is going to fit into a 'unsigned int', so 'idxVal' will never be (even on error) greater than that.
Incase of "virsh attach-device vm1 controller.xml --persistent" , the index value is -1 , as assigned by def->idx = -1; in virDomainControllerDefNew. The function virStrToLong_ui(idx, NULL, 10, &idxVal), returns the value UINT_MAX for idx = -1.
If the index must be non-negative, it looks like using virStrToLong_uip instead of virStrToLong_ui could work fine in rejecting negative values outright.
virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse controller index %s"), idx);
Considering this, I think the XML you used in your testsmost probably has index="-1" as attribute -- can you please confirm?
Thanks, -- Pino Toscano
On 11/14/2016 12:13 PM, Nitesh Konkar wrote:
In the current scenario the controller parsing fails when no index is specified .As the docs point out that specifying index is optional, libvirt is expected to fill the index without erroring out.
See: https://bugzilla.redhat.com/show_bug.cgi?id=1344899 Fixing this is not simple. I've tried to make a simple fix, and it's beyond that (see the BZ and all related email threads that it references for details).
Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- Before the Patch: cat controller.xml <controller model="virtio-scsi" type="scsi" />
virsh attach-device vm1 controller.xml --persistent error: Failed to attach device from controller.xml error: internal error: Cannot parse controller index -1
After the patch: virsh attach-device vm1 controller.xml --persistent Device attached successfully
Sure, it doesn't generate an error. But all you've done is make it ignore the fact that the index hasn't been set, you haven't actually set it. NACK.
--- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..c52e67f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8374,7 +8374,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (idx) { unsigned int idxVal; if (virStrToLong_ui(idx, NULL, 10, &idxVal) < 0 || - idxVal > INT_MAX) { + idxVal > UINT_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse controller index %s"), idx); goto error;
participants (3)
-
Laine Stump -
Nitesh Konkar -
Pino Toscano