Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API

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

>>> On 11/13/2015 at 01:00 AM, in message
<22084.50631.506663.212889@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
<Ian.Jackson@xxxxxxxxxxxxx> wrote: 
> Chun Yan Liu writes ("Re: [PATCH V8 3/7] libxl: add pvusb API"): 
> > On 11/10/2015 at 02:11 AM, in message 
> > <22080.57829.461049.37192@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson 
> > <Ian.Jackson@xxxxxxxxxxxxx> wrote:  
> > > Chunyan Liu writes ("[PATCH V8 3/7] libxl: add pvusb API"):  
> > > > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,  
> > > > +                               libxl_device_usbctrl *usbctrl,  
> > > > +                               libxl__ao_device *aodev)  
> > > > +{  
> > >   
> > > Thanks for adjusting the error-handling patterns in these functions.  
> > > The new way is good, except that:  
> > >   
> > > > +out:  
> > > > +    aodev->rc = rc;  
> > > > +    if (rc) aodev->callback(egc, aodev);  
> > >   
> > > Here, rc is always set, and indeed the code would be wrong if it were  
> > > not.  So can you remove the conditional please ?  Ie:  
> >  
> > Reading the codes, libxl__wait_device_connection will call 
> > aodev->callback properly. 
>  
> Indeed.  So you need to call aodev->callback() iff you don't call 
> libxl__wait_device_connection.  But libxl__wait_device_connection is 
> called only on the success exit path which ends in `return', not in 
> `goto out'.  So: 
>  
> > So here, only if (rc != 0), that means 
> > error happens, then we need to call aodev->callback to end the 
> > process. (Refer to current libxl__device_disk_add, all current code 
> > does similar work.) So I think the code is not wrong (?) 
>  
> In the `goto out' path, rc is always set.  Writing `if (rc)' implies 
> that it might not be (or is allowed not to be), which is misleading.

I'm convinced your suggestion is better :-). I'll adjust codes.
 
>  
>  
> > > > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid,  
> > > > +                                  uint8_t *bus, uint8_t *addr)  
> > > > +{  
> > > > +    char *filename;  
> > > > +    void *buf;  
> > > > +  
> > > > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);  
> > > > +    if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL))  
> > > > +        *bus = atoi((char *)buf);  
> > >   
> > > I don't think this cast (and the one for addr) are necessary ?  
> >  
> > Which cast? Here, we want to get a uint_8 value, but buf is a string, 
> > we need to do atoi. 
>  
> I mean that I think 
>  
>      +        *bus = atoi(buf);  
>  
> would compile and be correct.  buf would be automatically converted 
> from void* to const char*.  It's better to avoid casts where they are 
> not needed, because casts will suppress compiler warnings. 

Yes, you're right. Thanks very much!

>  
> For example if someone wanted to have the buffer be an updated 
> parameter they might do this: 
>  
>   static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid,  
> +                                   void **buf, 
>                                     uint8_t *bus, uint8_t *addr) 
> -    void *buf;  
>  
> and hope or expect the compiler to notice places where they had failed 
> to update the usage of buf.  With void **buf, 
>   atoi((char*)buf) 
> would compile and do a wrong thing whereas 
>   atoi(buf) 
> would produce a fatal warning. 
>  
> > > > +/* bind/unbind usb device interface */  
> > > > +static int unbind_usb_intf(libxl__gc *gc, char *intf, char **drvpath)  
> > > > +{  
> ... 
> > > > +        dp = libxl__zalloc(gc, PATH_MAX);  
> > > > +        dp = realpath(spath, dp);  
> > >   
> > > Why is this call to realpath needed ? 
> >  
> > In sysfs, /driver sometimes is a link, since we need to save the original 
> > driver to xenstore, so need to get the realpath of the driver. 
>  
> I mean, why is the path with all the symlinks in it not suitable for 
> storage and later use ? 

The symlink might be "../../../../../../bus/usb/drivers/btusb", we couldn't save
that to xenstore for later usage.

>  
>  
> > > > +static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb)  
> > > > +{  
> > > ...  
> > > > +        /* bind interface to its originial driver */  
> > > > +        drvpath = libxl__xs_read(gc, XBT_NULL,  
> > > > +                  GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",  
> > > > +                  usb_encode, usb_interface_xenstore_encode(intf)));  
> > > > +        if (drvpath && bind_usb_intf(gc, intf, drvpath))  
> > > > +            LOGE(WARN, "Couldn't bind %s to %s", intf, drvpath);  
> > >   
> > > This error message could be clearer.  
> > >   
> > > Also it would be worth a comment in the code to explain why this is a  
> > > warning (merely logged), rather than an error (logged and reported  
> > > with nonzero status to caller). 
> >  
> > Will update. When doing USB remove, it's the ideal result we can rebound 
> > the USB to its original driver, in case the USB interface failed to be 
> > rebound to its original driver, we will export warning but won't stop the 
> > removing process. 
>  
> Right.  Please put that in a comment :-). 
>  
>  
> > > > +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb)  
> > > > +{  
> > > ...  
> > > > +            if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 
> > > > 0)  
> {  
> > > > +                LOG(WARN, "Write of %s to node %s failed", drvpath,  
> path);  
> > > > +            }  
> > >   
> > > One of the advantages of libxl__xs_write_checked is that it logs  
> > > errors.  So you can leave that log message out.  
> > >   
> > > However, if the xs write fails, you should presumably stop, rather  
> > > than carrying on.  
> > >  
> > > I think you probably do things in the wrong order here.  You should  
> > > write the old driver info to xenstore first, so that if the bind to  
> > > usbback fails, you have the old driver info.  
> >  
> > Perhaps not. Once we finished adding entries to xenstore, pvusb 
> > frontend/backend drivers will detect the change and do probe work, if 
> > USB device is still not bound to USBBACK, there might be error. 
>  
> What I mean is this: 
>  
> Is it not possible to write the original path to xenstore before doing 
> the unbind ?  Otherwise it seems like there could be error paths where 
> the original path is not recorded, the xenstore write fails, and then 
> the information about how to rebind to the original driver has been 
> lost. 

I see. 

>  
> Does writing USBBACK_INFO_PATH"/%s/%s/driver_path" really trigger 
> usbback ? 

No, writing driver_path info to xenstore won't trigger usbback. Writing
frontend/backend info will.

>  
> > > > +    free(usb_encode);  
> > >   
> > > This came from the gc, I think.  
> ... 
> > > > +    switch (usbctrlinfo.type) {  
> > > > +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:  
> > > > +        LOG(ERROR, "Not supported");  
> > > > +        break;  
> > >   
> > > Needs to set rc.  
>  
> (Did you spot these comments ?  Normally I wouldn't even expect you to 
> quote the comments that you are just going to fix, but since you have 
> replied to the others, I thought I would check.)

No, it's definitely right, so I'll fix surely.
 
>  
>  
> > > > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,  
> > > > +                           libxl_device_usb *usb,  
> > > > +                           libxl__ao_device *aodev)  
> > > > +{  
> > > ...  
> > > > +        if (usbctrlinfo.backend_id != 0) {  
> > >   
> > > LIBXL_TOOLSTACK_DOMID please.  (And in remove, too.)  
> > >   
> > > Why is this check done in libxl__device_usb_add and do_usb_remove,  
> > > rather than in a symmetrical way? 
> >   
> > In libxl__device_usb_add, the set_default function implementation won't 
> > work if backend domain is USB driver domain in future, so we must add 
> > a check before set_default, so in libxl__device_usb_add. To check backend 
> > domain, we need to get usbctrl info. And in do_usb_add, to check usbctrl 
> > type, again we need to get usbctrl info. It's disgusting to do the same  
> thing 
> > twice. 
> >  
> > In usb remove, nothing in libxl__device_usb_remove is Dom0 specific, so 
> > I do all check in do_usb_remove, then only one time to get usbctrl info. 
>  
> Right. 
>  
> > If we merge libxl__device_usb_add and do_usb_add, then it's cleaner. 
>  
> See my comment below.  You've explained the distinction to my 
> satisfaction. 
>  
> But, to solve the duplication of the controller info acquisition, 
> perhaps you could have do_usb_add take the controller info as a 
> paramaeter ?

Yes, could be. Only do_usb_add and do_usb_remove parameters are not
symmetrical. 

>  
>  
> > > > +    /* Do the add */  
> > > > +    if (do_usb_add(gc, domid, usb, aodev)) {  
> > > > +        rc = ERROR_FAIL;  
> > > > +        goto out;  
> > > > +    }  
> > > > +  
> > > > +    libxl__ao_complete(egc, ao, 0);  
> > > > +    rc = 0;  
> > > > +  
> > > > +out:  
> > > > +    libxl_device_usbctrl_dispose(&usbctrl);  
> > > > +    libxl_usbctrlinfo_dispose(&usbctrlinfo);  
> > > > +    aodev->rc = rc;  
> > > > +    if (rc) aodev->callback(egc, aodev);  
> > > > +    return;  
> > >   
> > > Calling libxl__ao_complete here is quite wrong.  I think you should  
> > > call aodev->callback in both success and failure cases.  And it must  
> > > be the last thing you do.  
> >  
> > As mentioned in libxl__device_usbctrl_add, libxl__ao_complete will 
> > call callback, so in 'out' section, only if (rc !=0), which means 
> > error happens, then we need to call aodev->callback explicitly to 
> > end the process. 
> >  
> > Here, since we don't have actual async work needs to do, so call 
> > libxl__ao_complete(egc, ao, 0) directly to end the process. 
>  
> I'm afraid this is definiitely wrong.  There are a number of things 
> wrong with it, or a number of ways of looking at it: 
>  
> Most obviously, a single entrypoint should always complete its 
> execution the same way.  In this case the entrypoint takes an aodev 
> and in the error path calls aodev->callback().  So it should do the 
> same on the success path. 
>  
> Secondly (and relatedly), it is generally wrong for anything inside a 
> particular piece of machinery to terminate an ao. 
>  
> In this particular case I think it leads to a use-after-free bug: 
> completing the ao will (maybe) free the ao, and aodev was allocated 
> from the ao's gc.  So the write to aodev->rc is improper. 

Reasonable, I'll adjust codes. Thanks!

- Chunyan

>  
>  
> > > I might ask the same question about do_usb_add and  
> > > libxl__device_usb_add ?  
> >  
> > Using libxl__device_usb_add and within it extracting do_usb_add is to: 
> > * doing some initialization work and necessary check work in 
> >   libxl__device_usb_add (this part are common to PVUSB and QEMU USB) 
> > * doing actual adding usb operations in do_usb_add (this part will diverge 
> >   for PVUSB and QEMU USB) . 
>  
> This is a good reason for keeping them split.  Thanks for the 
> explanation. 
>  
>  
> Thanks for your other replies. 
>  
> Regards, 
> Ian. 
>  
>  


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