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