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