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

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 12/10/2015 at 08:08 PM, in message <56696B4B.7060801@xxxxxxxxxx>, George
Dunlap <george.dunlap@xxxxxxxxxx> wrote: 
> On 10/12/15 12:05, George Dunlap wrote: 
> > From: Chunyan Liu <cyliu@xxxxxxxx> 
> >  
> > 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 <cyliu@xxxxxxxx> 
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx> 
> > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx> 
>  
> 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 
>  
>

Attachment: 0004-libxl-pvusb-API-changes.PATCH
Description: Text document

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