Re: [PATCH] conf: Use UNIT_MAX for checking max value of unsigned int

[Nitesh Konkar <niteshkonkar.libvirt@xxxxxxxxx>]

On Mon, Nov 14, 2016 at 11:21 PM, Pino Toscano <ptoscano@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> ---

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
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list