11:13 a.m.
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(a)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
11:51 a.m.
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(a)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
12:25 p.m.
On Mon, Nov 14, 2016 at 11:21 PM, Pino Toscano <ptoscano(a)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(a)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
12:51 p.m.
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(a)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;
2930
days inactive
2930
days old
3 comments
3 participants
participants (3)
-
Laine Stump -
Nitesh Konkar -
Pino Toscano