|
[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/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"):
> > 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
>
> Thanks for this.
>
>
> I have reviewed it in detail (not just the ao handling aspects) and
> (I'm afraid) produced a large number of comments, relating to style,
> error handling, etc., as well as ao handling.
>
> Please let me know if anything I've said is unclear. I've been rather
> brief in my comments, rather than writing a paragraph for each one.
> Thanks.
>
>
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index dacfaae..a050e8b 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4218,11 +4218,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
> ...
> > +/* Macro for defining device remove/destroy functions for usbctrl */
> > +/* Following functions are defined:
> > + * libxl_device_usbctrl_remove
> > + * libxl_device_usbctrl_destroy
> > + */
> > +
> > +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f) \
>
> As George has said, I would prefer to avoid committing this
> duplication in-tree, even with a promise to de-duplicated it later.
>
>
> > +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. 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 (?)
>
> + aodev->callback(egc, aodev);
>
> The same goes for:
>
> > +static int
> > +libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, libxl_device_usb
> *usb);
> ...
>
> > + /* Remove usb devices first */
> > + rc = libxl__device_usb_list_for_usbctrl(gc, domid, usbctrl_devid,
> > + &usbs, &numusb);
> ^^
> While you're fixing other things, you could remove one of these space.s
>
> > +libxl_device_usbctrl *
> > +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
> > +{
> ...
> > + for (usbctrl = usbctrls; usbctrl < end;
> > + usbctrl++, entry++, (*num)++) {
>
> I think this would be clearer if there were a line break at the first
> semicolon too. Ie, I like
> for (A; B; C) {
> or
> for (A;
> B;
> C) {
>
> But I don't like very much
> for (A; B;
> C) {
> or
> for (A;
> B; C) {
>
> > +#define READ_SUBPATH(path, subpath) ({ \
> > + rc = libxl__xs_read_checked(gc, XBT_NULL, \
> > + GCSPRINTF("%s/" subpath, path), \
> > + &tmp); \
> > + if (rc) goto outerr; \
> > + (char *)tmp; \
> > + })
>
> Thanks, I'm pleased with how you have done this.
>
> > + be_path = READ_SUBPATH(fe_path, "backend");
> > + usbctrl->backend_domid = atoi(READ_SUBPATH(fe_path,
> "backend-id"));
> > + usbctrl->version = atoi(READ_SUBPATH(be_path, "usb-ver"));
> > + usbctrl->ports = atoi(READ_SUBPATH(be_path, "num-ports"));
>
> However, I have a question:
>
> Why do you use atoi here, but strtoul in libxl_device_usbctrl_getinfo ?
Didn't care much about that. atoi and strtoul both work. Previously use aoti,
in libxl_device_usbctrl_getinfo, to keep consistent with other functions (like
libxl_device_disk_getinfo), use strtoul.
Will update and use the same.
>
> > +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> > + libxl_device_usbctrl *usbctrl,
> > + libxl_usbctrlinfo *usbctrlinfo)
> > +{
> ...
> > + tmp = READ_SUBPATH(fe_path, "backend-id");
> > + usbctrlinfo->backend_id = tmp ? strtoul(tmp, NULL, 10) : -1;
>
> There are ten copies of this pattern with tmp and strtoul. I really
> think this needs to be refactored somehow. Can you please make a
> macro which returns the return value from strtoul ?
Will update.
>
> > +static char *usb_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
> > +{
> ...
> > + while ((de = readdir(dir))) {
>
> You need to use readdir_r, I'm afraid. Ordinary readdir is not
> threadsafe.
Yep. Thanks.
>
> > + }
> > +
> > + closedir(dir);
>
> This assumes that all errors from readdir are EOF. But that's not the
> case. Luckily readdir_r has more sensible error behaviour. But you
> do need to check separately for errors and EOF.
Will update.
>
> All in all this probably means that it would be clearer to write this
> as:
> for (;;) {
> r = readdir_r etc.
> if (r) ....
> if (!de) break;
>
> There is at least one more call to readdir in this patch which also
> needs to be fixed.
Yep.
>
> > +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.
>
> > +static int
> > +get_assigned_devices(libxl__gc *gc,
> > + libxl_device_usb **list, int *num)
> > +{
> ...
> > +out:
> > + if (rc) {
> > + *list = NULL;
> > + *num = 0;
> > + }
>
> Is this necessary ? Given that get_assigned_devices only has internal
> callers which expect gc'd memory, there is no memory cleanup issue.
> And a caller who gets a nonzero return value ought not to then look at
> the results.
>
> The only existing call site seems not to care.
>
> However, it would probably be better style to print a log message
> here, rather than in the caller.
Will update.
>
>
> > +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
> > +{
> ...
> > + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid);
> ^
> Our usual coding style would have a space here.
>
> > +static int
> > +libxl__device_usb_list_for_usbctrl(libxl__gc *gc, uint32_t domid,
> > + libxl_devid usbctrl,
> > + libxl_device_usb **usbs, int *num)
> ...
> > + for (i = 0; i < n; i++) {
> > + char *busid;
> > + libxl_device_usb *usb;
> > +
> > + busid = libxl__xs_read(gc, XBT_NULL,
> > + GCSPRINTF("%s/port/%d", be_path, i + 1));
>
> You should be using libxl__xs_read_checked here I think.
>
> This is true throughout. There are more calls to libxl__xs_read which
> probably need to be replaced.
Got it, will update all.
>
> ...
> > +out:
> > + if (rc) {
> > + *usbs = NULL;
> > + *num = 0;
> > + }
>
> As comment as before. However, the one call site for this ignores
> errors, which is a bug.
Will update.
>
>
> However. I think that get_assigned_devices has too much code that
> looks like the middle of libxl__device_usb_list_for_usbctrl and
> libxl_device_usb_list.
>
> I think you could write get_assigned_devices in terms of
> libxl_device_usb_list or maybe libxl__device_usb_list_for_usbctrl.
Will check and update.
>
>
> > + fe_path = GCSPRINTF("%s/device/vusb/%d",
> > + libxl__xs_get_dompath(gc, domid), usb->ctrl);
> > +
> > + rc = libxl__xs_read_checked(gc, XBT_NULL,
> > + GCSPRINTF("%s/backend", fe_path),
> &be_path);
> > + if (rc) goto out;
>
> After reading the backend path out of the frontend, you need to check
> that it's plausible. Otherwise there perhaps possible attacks where
> the frontend writes a backend path pointing somewhere else in
> xenstore.
>
> > + rc = libxl__xs_read_checked(gc, XBT_NULL,
> > + GCSPRINTF("%s/port/%d", be_path,
> usb->port),
> > + &tmp);
> > + if (rc) goto out;
> > +
> > + if (strcmp(tmp, "")) {
>
> This is rather odd. What happens if the path doesn't exist ? AFAICT
> this will segfault. (Another case in libxl_ctrlport_to_device_usb, at
> least.)
Hmm, we need to check tmp first, if (tmp && strcmp(tmp, ""))
>
> ....
> > +/* bind/unbind usb device interface */
> > +static int unbind_usb_intf(libxl__gc *gc, char *intf, char **drvpath)
> > +{
> > + char *path, *spath, *dp = NULL;
> > + int fd = -1;
> > + int rc;
> > + struct stat st;
> > +
> > + spath = GCSPRINTF(SYSFS_USB_DEV"/%s/driver", intf);
> > + if (!lstat(spath, &st)) {
>
> The code should not assume that all lstate failures are ENOENT.
Will update.
> > + 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.
>
> > + path = GCSPRINTF("%s/unbind", spath);
> > + fd = open(path, O_WRONLY);
> > + if (fd < 0)
> > + return ERROR_FAIL;
> > + rc = write(fd, intf, strlen(intf));
> > + close(fd);
> > + if (rc < 0)
> > + return ERROR_FAIL;
>
> All of these system calls should log errors (errno) appropriately.
> The write should check that the expected amount was written.
>
> rc should only be used for a libxl error code. See CODING_STYLE.
> (Multiple occurrences throughout this patch.)
Yep. Will update.
>
> Do we know that writes to sysfs don't ever return short or EINTR ?
>
> > +static int bind_usb_intf(libxl__gc *gc, char *intf, char *drvpath)
> > +{
>
> This function has a remarkable resemblance to much of the unbind
> function. The common code should be factored out.
Will check and update.
>
> > +static int usb_get_all_interfaces(libxl__gc *gc, libxl_device_usb *usb,
> > + char ***intfs, int *num)
> > +{
> > + DIR *dir;
>
> This (and the earlier function which also uses opendir) uses the wrong
> resource / error-handling pattern for dir. See ERROR HANDLING in
> CODING_SYTLE.
Will check and update.
>
> > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, a
> > + if (!busid) {
> > + rc = ERROR_FAIL;
>
> I think these internal functions such as usb_busaddr_to_busid, _bind,
> _is_assignable, etc., ought to return rc values, rather than all the
> callers inventing ERROR_FAIL.
I'll check.
>
>
> > + if (!(dir = opendir(SYSFS_USB_DEV))) {
> > + rc = ERROR_FAIL;
>
> Again, you need to log errno (and the path, too).
OK. Will update.
>
> > +/* Encode usb interface so that it could be written to xenstore as a key.
> > + *
> > + * Since xenstore key cannot include '.' or ':', we'll change '.' to '_',
> > + * change ':' to '-'. For example, 3-1:2.1 will be encoded to 3-1-2_1.
> > + * This will be used to save original driver of USB device to xenstore.
> > + */
>
> What is the syntax of the incoming busid ? Could it contain _ or - ?
> You should perhaps spot them and reject if it does.
The busid is in syntax like 3-1:2.1. It does contain '-'. But since we only use
the single direction, never decode back, so it won't harm.
>
> > +static char *usb_interface_xenstore_encode(char *busid)
> > +{
> > + char *str = strdup(busid);
>
> This function should either update the value in place, or be
> const-correct (ie, take a const char*).
>
> Do not use bare strdup. This means if it's going to make a copy, it
> needs to take a gc.
Will update.
>
>
> > +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.
>
> > +/* Bind USB device to "usbback" driver.
> > + *
> > + * If there are many interfaces under USB device, check each interface,
> > + * unbind from original driver and bind to "usbback" driver.
> > + */
> > +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.
>
> > + free(usb_encode);
>
> This came from the gc, I think.
>
>
> > +/*
> > + * USB add
> > + */
>
> Many of these comments often don't add much. OTOH I won't insist you
> remove them.
>
> > +static int do_usb_add(libxl__gc *gc, uint32_t domid,
> > + libxl_device_usb *usbdev,
> > + libxl__ao_device *aodev)
> > +{
> ...
> > + switch (usbctrlinfo.type) {
> > + case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> > + LOG(ERROR, "Not supported");
> > + break;
>
> Needs to set rc.
>
> > + case LIBXL_USBCTRL_TYPE_PV:
> > + rc = libxl__device_usb_add_xenstore(gc, domid, usbdev,
> > + aodev->update_json);
> > + if (rc) goto out;
> > +
> > + rc = usbback_dev_assign(gc, usbdev);
> > + if (rc) {
> > + libxl__device_usb_remove_xenstore(gc, domid, usbdev);
> > + goto out;
> > + }
> > + break;
> > + default:
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > +out:
>
> Before out, rc should be explicitly set to 0, rather than relying on
> the happenstance of the previous setting.
Yep.
>
> > +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.
If we merge libxl__device_usb_add and do_usb_add, then it's cleaner.
>
> > + /* 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.
>
> > +static int do_usb_remove(libxl__gc *gc, uint32_t domid,
> > + libxl_device_usb *usbdev)
> > +{
> > + libxl_ctx *ctx = CTX;
>
> What is this for ? I think, instead, you can just refer to CTX where
> needed.
Yep.
>
>
> ...
> > + switch (usbctrlinfo.type) {
> > + case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> > + LOG(ERROR, "Not supported");
> > + break;
> > + case LIBXL_USBCTRL_TYPE_PV:
> > + rc = libxl__device_usb_remove_xenstore(gc, domid, usbdev);
> > + if (rc) goto out;
> > +
> > + usbback_dev_unassign(gc, usbdev);
> > + break;
> > + default:
> > + rc = ERROR_FAIL;
> > + goto out;
>
> This `default' case should do something more sensible than simply
> returning ERROR_FAIL with no log message (for `add', too).
Will update.
>
> > +/* Operation to remove usb device.
> > + *
> > + * Generally, it does:
> > + * 1) check if the usb device is assigned to the domain
> > + * 2) remove the usb device from xenstore controller/port.
> > + * 3) unbind usb device from usbback and rebind to its original driver.
> > + * If usb device has many interfaces, do it to each interface.
> > + */
> > +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
> > + libxl_device_usb *usb)
> > +{
> > + int rc;
> > +
> > + if (usb->ctrl < 0 || usb->port < 1) {
> > + LOG(ERROR, "Invalid USB device");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + if (usb->devtype == LIBXL_USBDEV_TYPE_HOSTDEV &&
> > + (usb->u.hostdev.hostbus < 1 || usb->u.hostdev.hostaddr < 1)) {
> > + LOG(ERROR, "Invalid USB device of hostdev");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
>
> Why are these checks here rather than in do_usb_remove ?
>
> For that matter, why is this function not the same as do_usb_remove ?
>
> 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) .
Same to libxl__device_usb_remove and do_usb_remove.
Certainly, we could merge into one function if that is better. George may
have some opinions on this?
-Chunyan
>
>
> > +void libxl_device_usbctrl_list_free(libxl_device_usbctrl *list, int nr)
> > +{
> > + int i;
> > + for (i = 0; i < nr; i++)
> > + libxl_device_usbctrl_dispose(&list[i]);
> > + free(list);
> > +}
> > +
> > +void libxl_device_usb_list_free(libxl_device_usb *list, int nr)
> > +{
> > + int i;
> > + for (i = 0; i < nr; i++)
> > + libxl_device_usb_dispose(&list[i]);
> > + free(list);
> > +}
>
> It is a shame that these aren't autogenerated, but I won't ask you to
> arrange that (as there are already too many instances of this
> in-tree).
>
>
> Regards,
> Ian.
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |