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

[Nitesh Konkar]

On Mon, Nov 14, 2016 at 11:21 PM, Pino Toscano <ptoscano at 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 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.
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161114/60755192/attachment-0001.htm>