[PATCH 0/4] pvusb fixes

[PATCH 0/4] pvusb fixes

[Chunyan Liu]

Patch series of pvusb fixes.

Chunyan Liu (4):
  a fix in libxl_device_usbdev_list
  correct libxl_write_exactly sizeof
  cleanup redundant lstat in libxl_pvusb.c
  fix a pvusb type check

 tools/libxl/libxl_pvusb.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 1/4] a fix in libxl_device_usbdev_list

[Chunyan Liu]

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(-)

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));
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] a fix in libxl_device_usbdev_list

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] a fix in libxl_device_usbdev_list

[Chun Yan Liu]

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] a fix in libxl_device_usbdev_list

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] a fix in libxl_device_usbdev_list

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] a fix in libxl_device_usbdev_list

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/4] correct libxl_write_exactly sizeof

[Chunyan Liu]

sizeof is wrongly used in libxl_write_exactly function, using
strlen instead.

CID: 1358110
CID: 1358109

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 04e41b4..45117cf 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -1025,7 +1025,7 @@ static int unbind_usbintf(libxl__gc *gc, const char *intf)
         goto out;
     }
 
-    if (libxl_write_exactly(CTX, fd, intf, sizeof(intf), path, intf)) {
+    if (libxl_write_exactly(CTX, fd, intf, strlen(intf), path, intf)) {
         rc = ERROR_FAIL;
         goto out;
     }
@@ -1065,7 +1065,7 @@ static int bind_usbintf(libxl__gc *gc, const char *intf, const char *drvpath)
         goto out;
     }
 
-    if (libxl_write_exactly(CTX, fd, intf, sizeof(intf), path, intf)) {
+    if (libxl_write_exactly(CTX, fd, intf, strlen(intf), path, intf)) {
         rc = ERROR_FAIL;
         goto out;
     }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/4] correct libxl_write_exactly sizeof

[Ian Jackson]

Chunyan Liu writes ("[PATCH 2/4] correct libxl_write_exactly sizeof"):
> sizeof is wrongly used in libxl_write_exactly function, using
> strlen instead.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/4] cleanup redundant lstat in libxl_pvusb.c

[Chunyan Liu]

CID: 1358112

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 | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 45117cf..02d3e55 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -983,25 +983,14 @@ static char *usbdev_busid_from_ctrlport(libxl__gc *gc, uint32_t domid,
 static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char **drvpath)
 {
     char *spath, *dp = NULL;
-    struct stat st;
-    int r;
 
     spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
 
-    r = lstat(spath, &st);
-    if (r == 0) {
-        /* Find the canonical path to the driver. */
-        dp = libxl__zalloc(gc, PATH_MAX);
-        dp = realpath(spath, dp);
-        if (!dp) {
-            LOGE(ERROR, "get realpath failed: '%s'", spath);
-            return ERROR_FAIL;
-        }
-    } else if (errno == ENOENT) {
-        /* driver path doesn't exist */
-        dp = NULL;
-    } else {
-        LOGE(ERROR, "lstat failed: '%s'", spath);
+    /* Find the canonical path to the driver. */
+    dp = libxl__zalloc(gc, PATH_MAX);
+    dp = realpath(spath, dp);
+    if (!dp && errno != ENOENT) {
+        LOGE(ERROR, "get realpath failed: '%s'", spath);
         return ERROR_FAIL;
     }
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] cleanup redundant lstat in libxl_pvusb.c

[Ian Jackson]

Chunyan Liu writes ("[PATCH 3/4] cleanup redundant lstat in libxl_pvusb.c"):
> CID: 1358112
> 
> 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>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Although for this and the previous patch, it would have been better to
add "libxl:" to the start of the Subject.

I will fix that up as I commit.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 4/4] fix a pvusb type check

[Chunyan Liu]

Missing a check of controller type.

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>
CC: Juergen Gross <jgross@suse.com>
---
This affects Juergen's qusb patch too. Fix that together. This patch
could be applied on top of Juergen's qusb backend patch.

 tools/libxl/libxl_pvusb.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 02d3e55..6447639 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -811,6 +811,13 @@ static int libxl__device_usbdev_setdefault(libxl__gc *gc,
                 }
             }
 
+            if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
+                usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
+                LOG(ERROR, "Unsupported USB controller type");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+
             rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
                                                     update_json);
             if (rc) goto out;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 4/4] fix a pvusb type check

[Juergen Gross]

On 07/04/16 11:40, Chunyan Liu wrote:
> Missing a check of controller type.
> 
> 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>
> CC: Juergen Gross <jgross@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
> This affects Juergen's qusb patch too. Fix that together. This patch
> could be applied on top of Juergen's qusb backend patch.
> 
>  tools/libxl/libxl_pvusb.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
> index 02d3e55..6447639 100644
> --- a/tools/libxl/libxl_pvusb.c
> +++ b/tools/libxl/libxl_pvusb.c
> @@ -811,6 +811,13 @@ static int libxl__device_usbdev_setdefault(libxl__gc *gc,
>                  }
>              }
>  
> +            if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
> +                usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
> +                LOG(ERROR, "Unsupported USB controller type");
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +
>              rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
>                                                      update_json);
>              if (rc) goto out;
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel