[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |