subject:"\[Xen\-devel\] \[PATCH v10 3\/5\] libxl\: add pvusb API"

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

2015-12-14 Thread Chun Yan Liu



>>> On 12/14/2015 at 07:37 PM, in message
, George
Dunlap  wrote: 
> On Mon, Dec 14, 2015 at 7:25 AM, Chun Yan Liu  wrote: 
> > 
> > 
>  On 12/10/2015 at 08:08 PM, in message <56696b4b.7060...@citrix.com>, 
>  George 
> > Dunlap  wrote: 
> >> On 10/12/15 12:05, George Dunlap wrote: 
> >> > From: Chunyan Liu  
> >> > 
> >> > Add pvusb APIs, including: 
> >> >  - attach/detach (create/destroy) virtual usb controller. 
> >> >  - attach/detach usb device 
> >> >  - list usb controller and usb devices 
> >> >  - some other helper functions 
> >> > 
> >> > Signed-off-by: Chunyan Liu  
> >> > Signed-off-by: Simon Cao  
> >> > Signed-off-by: George Dunlap  
> >> 
> >> Attached is a diff of v9 -> v10 for convenience. 
> > 
> > Thanks very much, George! 
> > I've applied your new patch and tested, there are a couple of changes  
> needed to 
> > get tests PASSED. A small extra patch is written on top of your new patch,  
> as in 
> > attachment, please have a look. 
>  
> Thanks -- the changes in the patch look good. 
>  
> >> > +static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid, 
> >> > + char ***intfs, int *num) 
> >> > +{ 
> >> > +DIR *dir; 
> >> > +char *buf; 
> >> > +int rc; 
> >> > + 
> >> > +*intfs = NULL; 
> >> > +*num = 0; 
> >> > + 
> >> > +buf = GCSPRINTF("%s:", busid); 
> >> > + 
> >> > +dir = opendir(SYSFS_USB_DEV); 
> >> > +if (!dir) { 
> >> > +LOGE(ERROR, "opendir failed: '%s'", SYSFS_USB_DEV); 
> >> > +return ERROR_FAIL; 
> >> > +} 
> >> > + 
> >> > +size_t need = offsetof(struct dirent, d_name) + 
> >> > +pathconf(SYSFS_USB_DEV, _PC_NAME_MAX) + 1; 
> >> > +struct dirent *de_buf = libxl__zalloc(gc, need); 
> >> 
> >> Is this thing with manually calculating the size of the structure really 
> >> necessary?  Could we not just declare "struct dirent de_buf" on the stack? 
> > 
> > Calculating in above way is to allocate enough space for d_name, whereas 
> > "struct dirent de_buf" won't allocate space for d_name (which is char *). 
> > 
> > Codes for calling read_dir_r are often done like above. 
>  
> OK -- in that case, can you put the allocation of the structure into a 
> macro or helper function, fold in the patch you sent, and re-send this 
> series as v11? 

OK. Will update soon!

- Chunyan

>  
> Thanks! 
>  
>  -George 
>  
>  



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

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

2015-12-14 Thread George Dunlap

On Mon, Dec 14, 2015 at 7:25 AM, Chun Yan Liu  wrote:
>
>
 On 12/10/2015 at 08:08 PM, in message <56696b4b.7060...@citrix.com>, George
> Dunlap  wrote:
>> On 10/12/15 12:05, George Dunlap wrote:
>> > From: Chunyan Liu 
>> >
>> > Add pvusb APIs, including:
>> >  - attach/detach (create/destroy) virtual usb controller.
>> >  - attach/detach usb device
>> >  - list usb controller and usb devices
>> >  - some other helper functions
>> >
>> > Signed-off-by: Chunyan Liu 
>> > Signed-off-by: Simon Cao 
>> > Signed-off-by: George Dunlap 
>>
>> Attached is a diff of v9 -> v10 for convenience.
>
> Thanks very much, George!
> I've applied your new patch and tested, there are a couple of changes needed 
> to
> get tests PASSED. A small extra patch is written on top of your new patch, as 
> in
> attachment, please have a look.

Thanks -- the changes in the patch look good.

>> > +static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid,
>> > + char ***intfs, int *num)
>> > +{
>> > +DIR *dir;
>> > +char *buf;
>> > +int rc;
>> > +
>> > +*intfs = NULL;
>> > +*num = 0;
>> > +
>> > +buf = GCSPRINTF("%s:", busid);
>> > +
>> > +dir = opendir(SYSFS_USB_DEV);
>> > +if (!dir) {
>> > +LOGE(ERROR, "opendir failed: '%s'", SYSFS_USB_DEV);
>> > +return ERROR_FAIL;
>> > +}
>> > +
>> > +size_t need = offsetof(struct dirent, d_name) +
>> > +pathconf(SYSFS_USB_DEV, _PC_NAME_MAX) + 1;
>> > +struct dirent *de_buf = libxl__zalloc(gc, need);
>>
>> Is this thing with manually calculating the size of the structure really
>> necessary?  Could we not just declare "struct dirent de_buf" on the stack?
>
> Calculating in above way is to allocate enough space for d_name, whereas
> "struct dirent de_buf" won't allocate space for d_name (which is char *).
>
> Codes for calling read_dir_r are often done like above.

OK -- in that case, can you put the allocation of the structure into a
macro or helper function, fold in the patch you sent, and re-send this
series as v11?

Thanks!

 -George

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

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

2015-12-13 Thread Chun Yan Liu



>>> On 12/10/2015 at 08:08 PM, in message <56696b4b.7060...@citrix.com>, George
Dunlap  wrote: 
> On 10/12/15 12:05, George Dunlap wrote: 
> > From: Chunyan Liu  
> >  
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list usb controller and usb devices 
> >  - some other helper functions 
> >  
> > Signed-off-by: Chunyan Liu  
> > Signed-off-by: Simon Cao  
> > Signed-off-by: George Dunlap  
>  
> Attached is a diff of v9 -> v10 for convenience. 

Thanks very much, George!
I've applied your new patch and tested, there are a couple of changes needed to
get tests PASSED. A small extra patch is written on top of your new patch, as in
attachment, please have a look.

Otherwise, I agreed with all your other changes. It's great improvement.
Thanks a lot!  

>  
> One remaining question I had regarding this patch... 

For the question, see below.

>  
> > +static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid, 
> > + char ***intfs, int *num) 
> > +{ 
> > +DIR *dir; 
> > +char *buf; 
> > +int rc; 
> > + 
> > +*intfs = NULL; 
> > +*num = 0; 
> > + 
> > +buf = GCSPRINTF("%s:", busid); 
> > + 
> > +dir = opendir(SYSFS_USB_DEV); 
> > +if (!dir) { 
> > +LOGE(ERROR, "opendir failed: '%s'", SYSFS_USB_DEV); 
> > +return ERROR_FAIL; 
> > +} 
> > + 
> > +size_t need = offsetof(struct dirent, d_name) + 
> > +pathconf(SYSFS_USB_DEV, _PC_NAME_MAX) + 1; 
> > +struct dirent *de_buf = libxl__zalloc(gc, need); 
>  
> Is this thing with manually calculating the size of the structure really 
> necessary?  Could we not just declare "struct dirent de_buf" on the stack?

Calculating in above way is to allocate enough space for d_name, whereas
"struct dirent de_buf" won't allocate space for d_name (which is char *).

Codes for calling read_dir_r are often done like above.

- Chunyan

>  
> If it is necessary, it would be better to have it inside a function or 
> macro called "alloc_dirent" or something like that. 
>  
>  -George 
>  
>  


>From 771c99218a4704eb6a4850dfafb1cafad0798b9d Mon Sep 17 00:00:00 2001
From: Chunyan Liu 
Date: Mon, 14 Dec 2015 15:00:50 +0800
Subject: [PATCH 4/6] libxl pvusb API changes

* format fix: extra white space, line > 80, etc.
* return ERROR_FAILED instead of errno (>0) in sysfs_write_intf
* fix an error in libxl_ctrlport_to_device_usbdev

Signed-off-by: Chunyan Liu 
---
 tools/libxl/libxl_pvusb.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index cb25fa8..5f189d6 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -287,14 +287,15 @@ out:
 return;
 }
 
-static const char * vusb_be_from_xs_fe(libxl__gc *gc, const char *fe_path,
-   uint32_t tgt_domid) {
+static const char *vusb_be_from_xs_fe(libxl__gc *gc, const char *fe_path,
+  uint32_t tgt_domid)
+{
 const char *be_path;
 int r;
 uint32_t be_domid, fe_domid;
-
+
 r = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/backend", fe_path),
- _path);
+   _path);
 if (r || !be_path) return NULL;
 
 /* Check to see that it has the proper form, and that fe_domid ==
@@ -302,11 +303,11 @@ static const char * vusb_be_from_xs_fe(libxl__gc *gc, 
const char *fe_path,
 r = sscanf(be_path, "/local/domain/%d/backend/vusb/%d",
_domid, _domid);
 
-if ( r != 2 || fe_domid != tgt_domid ) {
+if (r != 2 || fe_domid != tgt_domid) {
 LOG(ERROR, "Malformed backend, refusing to use");
 return NULL;
 }
-
+
 return be_path;
 }
 
@@ -810,7 +811,7 @@ static int libxl__device_usbdev_setdefault(libxl__gc *gc,
 } else {
 /* A controller was specified; look it up */
 const char *fe_path, *be_path, *tmp;
-
+
 fe_path = GCSPRINTF("%s/device/vusb/%d",
 libxl__xs_get_dompath(gc, domid),
 usbdev->ctrl);
@@ -828,7 +829,7 @@ static int libxl__device_usbdev_setdefault(libxl__gc *gc,
   be_path, usbdev->port),
 );
 if (rc) goto out;
-
+
 if (tmp && strcmp(tmp, "")) {
 LOG(ERROR, "The controller port isn't available");
 rc = ERROR_FAIL;
@@ -837,7 +838,7 @@ static int libxl__device_usbdev_setdefault(libxl__gc *gc,
 } else {
 /* No port was requested. Choose free port. */
 int i, ports;
-
+
 rc =

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

2015-12-10 Thread George Dunlap

On 10/12/15 12:05, George Dunlap wrote:
> From: Chunyan Liu 
> 
> Add pvusb APIs, including:
>  - attach/detach (create/destroy) virtual usb controller.
>  - attach/detach usb device
>  - list usb controller and usb devices
>  - some other helper functions
> 
> Signed-off-by: Chunyan Liu 
> Signed-off-by: Simon Cao 
> Signed-off-by: George Dunlap 

Attached is a diff of v9 -> v10 for convenience.

One remaining question I had regarding this patch...

> +static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid,
> + char ***intfs, int *num)
> +{
> +DIR *dir;
> +char *buf;
> +int rc;
> +
> +*intfs = NULL;
> +*num = 0;
> +
> +buf = GCSPRINTF("%s:", busid);
> +
> +dir = opendir(SYSFS_USB_DEV);
> +if (!dir) {
> +LOGE(ERROR, "opendir failed: '%s'", SYSFS_USB_DEV);
> +return ERROR_FAIL;
> +}
> +
> +size_t need = offsetof(struct dirent, d_name) +
> +pathconf(SYSFS_USB_DEV, _PC_NAME_MAX) + 1;
> +struct dirent *de_buf = libxl__zalloc(gc, need);

Is this thing with manually calculating the size of the structure really
necessary?  Could we not just declare "struct dirent de_buf" on the stack?

If it is necessary, it would be better to have it inside a function or
macro called "alloc_dirent" or something like that.

 -George

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a479465..26cd5fa 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3203,7 +3203,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
 aodev->dev = device;
 aodev->callback = local_device_detach_cb;
 aodev->force = 0;
-libxl__initiate_device_remove(egc, aodev);
+libxl__initiate_device_generic_remove(egc, aodev);
 return;
 }
 
@@ -4144,36 +4144,6 @@ out:
 return rc;
 }
 
-static void libxl__initiate_device_disk_remove(libxl__egc *egc,
-   libxl__ao_device *aodev)
-{
-return libxl__initiate_device_remove(egc, aodev);
-}
-
-static void libxl__initiate_device_nic_remove(libxl__egc *egc,
-  libxl__ao_device *aodev)
-{
-return libxl__initiate_device_remove(egc, aodev);
-}
-
-static void libxl__initiate_device_vtpm_remove(libxl__egc *egc,
-   libxl__ao_device *aodev)
-{
-return libxl__initiate_device_remove(egc, aodev);
-}
-
-static void libxl__initiate_device_vkb_remove(libxl__egc *egc,
-  libxl__ao_device *aodev)
-{
-return libxl__initiate_device_remove(egc, aodev);
-}
-
-static void libxl__initiate_device_vfb_remove(libxl__egc *egc,
-  libxl__ao_device *aodev)
-{
-return libxl__initiate_device_remove(egc, aodev);
-}
-
 /**/
 
 /* Macro for defining device remove/destroy functions in a compact way */
@@ -4191,7 +4161,7 @@ static void libxl__initiate_device_vfb_remove(libxl__egc *egc,
  * libxl_device_usbctrl_remove
  * libxl_device_usbctrl_destroy
  */
-#define DEFINE_DEVICE_REMOVE(type, removedestroy, f)\
+#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\
 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
 uint32_t domid, libxl_device_##type *type,  \
 const libxl_asyncop_how *ao_how)\
@@ -4211,13 +4181,19 @@ static void libxl__initiate_device_vfb_remove(libxl__egc *egc,
 aodev->dev = device;\
 aodev->callback = device_addrm_aocomplete;  \
 aodev->force = f;   \
-libxl__initiate_device_##type##_remove(egc, aodev); \
+libxl__initiate_device_##remtype##_remove(egc, aodev);  \
 \
 out:\
-if (rc) return AO_CREATE_FAIL(rc);\
+if (rc) return AO_CREATE_FAIL(rc);  \
 return AO_INPROGRESS;   \
 }
 
+#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
+DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f)
+
+#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f)  \
+DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f)
+
 /* Define all remove/destroy functions and undef the macro */
 
 /* disk */
@@ -4242,8 +4218,8 @@ DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
 DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
 /* usbctrl */
-DEFINE_DEVICE_REMOVE(usbctrl, remove, 0)

[Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

2015-12-10 Thread George Dunlap

From: Chunyan Liu 

Add pvusb APIs, including:
 - attach/detach (create/destroy) virtual usb controller.
 - attach/detach usb device
 - list usb controller and usb devices
 - some other helper functions

Signed-off-by: Chunyan Liu 
Signed-off-by: Simon Cao 
Signed-off-by: George Dunlap 
---
Changes since v9:
- Rework DEFINE_DEVICE_REMOVE
- Got rid of redundant local ctx variable
- Got rid of return [void function] when returning from a void function
- Added spaces between STRING_MACRO and "x", as requested by IanJ when 
reviewing v8
- Got rid of another unnecessary void* -> char* cast
- Make vusb_be_from_xs_fe function to read a backend from a front end and check 
it for sanity
- Refactor libxl__device_usbdev_setdefault() to avoid code duplication for 
{ctrl,port} and {ctrl,NULL} case

CC: Ian Campbell 
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/libxl/Makefile |2 +-
 tools/libxl/libxl.c  |   34 +-
 tools/libxl/libxl.h  |   77 ++
 tools/libxl/libxl_device.c   |   13 +-
 tools/libxl/libxl_internal.h |   22 +-
 tools/libxl/libxl_osdeps.h   |   13 +
 tools/libxl/libxl_pvusb.c| 1542 ++
 tools/libxl/libxl_types.idl  |   46 +
 tools/libxl/libxl_types_internal.idl |1 +
 tools/libxl/libxl_utils.c|   18 +
 tools/libxl/libxl_utils.h|5 +
 11 files changed, 1760 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 6ff5bee..a36145a 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -103,7 +103,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_stream_read.o libxl_stream_write.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
libxl_qmp.o libxl_event.o libxl_fork.o \
-   libxl_dom_suspend.o $(LIBXL_OBJS-y)
+   libxl_dom_suspend.o libxl_pvusb.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 36dc37d..0485b04 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3201,7 +3201,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc 
*egc,
 aodev->dev = device;
 aodev->callback = local_device_detach_cb;
 aodev->force = 0;
-libxl__initiate_device_remove(egc, aodev);
+libxl__initiate_device_generic_remove(egc, aodev);
 return;
 }
 
@@ -4154,8 +4154,10 @@ out:
  * libxl_device_vkb_destroy
  * libxl_device_vfb_remove
  * libxl_device_vfb_destroy
+ * libxl_device_usbctrl_remove
+ * libxl_device_usbctrl_destroy
  */
-#define DEFINE_DEVICE_REMOVE(type, removedestroy, f)\
+#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\
 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
 uint32_t domid, libxl_device_##type *type,  \
 const libxl_asyncop_how *ao_how)\
@@ -4175,13 +4177,19 @@ out:
 aodev->dev = device;\
 aodev->callback = device_addrm_aocomplete;  \
 aodev->force = f;   \
-libxl__initiate_device_remove(egc, aodev);  \
+libxl__initiate_device_##remtype##_remove(egc, aodev);  \
 \
 out:\
-if (rc) return AO_CREATE_FAIL(rc);\
+if (rc) return AO_CREATE_FAIL(rc);  \
 return AO_INPROGRESS;   \
 }
 
+#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
+DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f)
+
+#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f)  \
+DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f)
+
 /* Define all remove/destroy functions and undef the macro */
 
 /* disk */
@@ -4205,6 +4213,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
 DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
 DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
+/* usbctrl */
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
+
 /* channel/console hotunplug is not implemented. There are 2 possibilities:
  * 1. add support for secondary consoles to xenconsoled
  * 2. dynamically add/remove qemu chardevs via qmp messages. */
@@ -4218,6 +4230,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
  * libxl_device_disk_add
  *

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

Re: [Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

[Xen-devel] [PATCH v10 3/5] libxl: add pvusb API

5 matches

Site Navigation

Mail list logo

Footer information