<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Nov 14, 2016 at 11:21 PM, Pino Toscano <<a href="mailto:ptoscano@redhat.com" target="_blank">ptoscano@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 <<a href="mailto:nitkon12@linux.vnet.ibm.com">nitkon12@linux.vnet.ibm.com</a>> > --- 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. </blockquote><div>As documented here <a href="https://libvirt.org/formatdomain.html#elementsControllers">https://libvirt.org/formatdomain.html#elementsControllers</a> , 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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > virsh attach-device vm1 controller.xml --persistent > error: Failed to attach device from controller.xml > error: internal error: Cannot parse controller index -1 </blockquote><div> virsh attach-device vm1 controller.xml --live and virsh attach-device vm1 controller.xml --config work fine. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > 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. </blockquote><div>Incase of "virsh attach-device vm1 controller.xml --persistent" , the index value </div><div>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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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</blockquote></div> </div></div>