| Message ID | 1460022028-25174-2-git-send-email-cyliu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chunyan Liu writes ("[PATCH 1/4] a fix in libxl_device_usbdev_list"):
> In testing with libvirt pvusb functionality, found a rc check
> error in libxl_device_usbdev_list. Correct it.
Thanks. But now that I look at this code I'm not sure your fix is
complete.
> diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
> index 5f92628..04e41b4 100644
> --- a/tools/libxl/libxl_pvusb.c
> +++ b/tools/libxl/libxl_pvusb.c
> @@ -701,13 +701,13 @@ libxl_device_usbdev_list(libxl_ctx *ctx, uint32_t domid, int *num)
> usbctrls = libxl__xs_directory(gc, XBT_NULL, path, &nc);
>
> for (i = 0; i < nc; i++) {
> - int r, nd = 0;
> + int rc, nd = 0;
> libxl_device_usbdev *tmp = NULL;
>
> - r = libxl__device_usbdev_list_for_usbctrl(gc, domid,
> + rc = libxl__device_usbdev_list_for_usbctrl(gc, domid,
> atoi(usbctrls[i]),
> &tmp, &nd);
> - if (!r || !nd) continue;
> + if (rc || !nd) continue;
If libxl__device_usbdev_list_for_usbctrl fails, shouldn't
libxl_device_usbdev_list fail too ?
Ian.
Chunyan Liu writes ("[PATCH 1/4] a fix in libxl_device_usbdev_list"):
> In testing with libvirt pvusb functionality, found a rc check
> error in libxl_device_usbdev_list. Correct it. This function
> is not used by xl.
Thanks for these. I have applied 2,3, and 4. But not 1, as I had
comments on it.
Regards,
Ian.
>>> On 4/8/2016 at 12:45 AM, in message <22278.36492.245114.295391@mariner.uk.xensource.com>, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Chunyan Liu writes ("[PATCH 1/4] a fix in libxl_device_usbdev_list"): > > In testing with libvirt pvusb functionality, found a rc check > > error in libxl_device_usbdev_list. Correct it. > > Thanks. But now that I look at this code I'm not sure your fix is > complete. > > > diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c > > index 5f92628..04e41b4 100644 > > --- a/tools/libxl/libxl_pvusb.c > > +++ b/tools/libxl/libxl_pvusb.c > > @@ -701,13 +701,13 @@ libxl_device_usbdev_list(libxl_ctx *ctx, uint32_t > domid, int *num) > > usbctrls = libxl__xs_directory(gc, XBT_NULL, path, &nc); > > > > for (i = 0; i < nc; i++) { > > - int r, nd = 0; > > + int rc, nd = 0; > > libxl_device_usbdev *tmp = NULL; > > > > - r = libxl__device_usbdev_list_for_usbctrl(gc, domid, > > + rc = libxl__device_usbdev_list_for_usbctrl(gc, domid, > > atoi(usbctrls[i]), > > &tmp, &nd); > > - if (!r || !nd) continue; > > + if (rc || !nd) continue; > > If libxl__device_usbdev_list_for_usbctrl fails, shouldn't > libxl_device_usbdev_list fail too ? Following the similar definitions of other device types, the return value of this function is "libxl_device_usbdev *", to treat the above case as failure, it cannot be reflected through return value, we can only set return value to NULL, but that will be confusing with a real no-device case. - Chunyan > > Ian. > >
Chun Yan Liu writes ("Re: [PATCH 1/4] a fix in libxl_device_usbdev_list"):
> On 4/8/2016 at 12:45 AM, in message
> <22278.36492.245114.295391@mariner.uk.xensource.com>, Ian Jackson
> <Ian.Jackson@eu.citrix.com> wrote:
> > If libxl__device_usbdev_list_for_usbctrl fails, shouldn't
> > libxl_device_usbdev_list fail too ?
>
> Following the similar definitions of other device types, the return value of
> this function is "libxl_device_usbdev *", to treat the above case as failure,
> it cannot be reflected through return value, we can only set return value
> to NULL, but that will be confusing with a real no-device case.
Urk. Yes, you're right. *sigh*
We won't fix that bad API pattern for 4.7. So
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
Ian Jackson writes ("Re: [PATCH 1/4] a fix in libxl_device_usbdev_list"):
> We won't fix that bad API pattern for 4.7. So
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Queued.
Ian.
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c index 5f92628..04e41b4 100644 --- a/tools/libxl/libxl_pvusb.c +++ b/tools/libxl/libxl_pvusb.c @@ -701,13 +701,13 @@ libxl_device_usbdev_list(libxl_ctx *ctx, uint32_t domid, int *num) usbctrls = libxl__xs_directory(gc, XBT_NULL, path, &nc); for (i = 0; i < nc; i++) { - int r, nd = 0; + int rc, nd = 0; libxl_device_usbdev *tmp = NULL; - r = libxl__device_usbdev_list_for_usbctrl(gc, domid, + rc = libxl__device_usbdev_list_for_usbctrl(gc, domid, atoi(usbctrls[i]), &tmp, &nd); - if (!r || !nd) continue; + if (rc || !nd) continue; usbdevs = libxl__realloc(NOGC, usbdevs, sizeof(*usbdevs) * (*num + nd));
In testing with libvirt pvusb functionality, found a rc check error in libxl_device_usbdev_list. Correct it. This function is not used by xl. Signed-off-by: Chunyan Liu <cyliu@suse.com> CC: Simon Cao <caobosimon@gmail.com> CC: George Dunlap <george.dunlap@citrix.com> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> --- tools/libxl/libxl_pvusb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)