On Mon, Nov 14, 2016 at 11:21 PM, Pino Toscano <[email protected]> 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 <[email protected]> > > --- > > 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
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
