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

[Pino Toscano]

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 at 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.

> virsh attach-device vm1 controller.xml  --persistent
> error: Failed to attach device from controller.xml
> error: internal error: Cannot parse controller index -1
> 
> 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161114/2debc05e/attachment-0001.sig>