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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Chunyan Liu writes ("[PATCH V7 3/7] libxl: add pvusb API"):
> Add pvusb APIs, including:
...

> +/* Utility to read backend xenstore keys */
> +#define READ_BACKEND(tgc, subpath)                                    \
> +            libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath, be_path))
> +
> +/* Utility to read frontend xenstore keys */
> +#define READ_FRONTEND(tgc, subpath)                                   \
> +            libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath, fe_path))
> +

Thanks for trying to reduce repetition.  But I think these macros need
to be improved.  I'm am not particularly keen on them in this form,
for a number of reasons.

 * They implicitly rely on variables (be_path and fe_path) being in
   scope but there is no associated doc comment.  That would be OK if
   they were macros defined within a single function and #undef'd
   before the function end.

 * Their functionality is not really usb-specific.  If this is a good
   thing to do they should be in libxl_internal.h.

 * They should use libxl__xs_read_checked.  That means they should
   probably also implicitly assume that rc is in scope, and `goto out'
   on error.  (There are many calls to libxl__xs_read here to which
   the same observation applies.)

 * I think there is no reason for these to take a `tgc' parameter.
   All of the call sites pass exactly `gc'.  If these macros are going
   to make assumptions about what's in their scope, `gc' is a very
   good candidate (which many existing macros rely on).

You can go two routes with this: make much more local macros, and
#undef them.  Or formalise these macros and widen their scope to the
whole of libxl.  I think the latter would probably be best.

Perhaps something like:

/*
 * const char *READ_SUBPATH(const char *febe_path, const char *subpath);
 *
 * Expects in scope:
 *    int rc;
 *    libxl__gc *gc;
 *    out:
 *
 * Reads febe_path/subpath from xenstore.  Does not use a transaction.
 * On xenstore errors, sets rc and does `goto out'.
 * If the path does not exist, returns NULL.
 */
#define READ_SUBPATH(febe_path, subpath) ...

What do you think ?


> +/* AO operation to add a usb controller.
> + *
> + * Generally, it does:
> + * 1) fill in necessary usb controler information with default value
> + * 2) write usb controller frontend/backend info to xenstore, update json
> + *    config file if necessary.
> + * 3) wait for device connection. PVUSB frontend and backend driver will
> + *    probe xenstore paths and build connection between frontend and backend.
> + */
> +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
> +                               libxl_device_usbctrl *usbctrl,
> +                               libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    libxl__device *device;
> +    int rc;
> +
> +    rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl);
> +    if (rc < 0) goto out;
> +
> +    if (usbctrl->devid == -1) {
> +        usbctrl->devid = libxl__device_nextid(gc, domid, "vusb");
> +        if (usbctrl->devid < 0) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }
> +
> +    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV) {
> +        LOG(ERROR, "Unsupported USB controller type");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
> +                                            aodev->update_json);
> +    if (rc) goto out;
> +
> +    GCNEW(device);
> +    rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
> +    if (rc) goto out;
> +
> +    aodev->dev = device;
> +    aodev->action = LIBXL__DEVICE_ACTION_ADD;
> +    libxl__wait_device_connection(egc, aodev);
> +    rc = 0;
> +
> +out:
> +    aodev->rc = rc;
> +    if (rc) aodev->callback(egc, aodev);

This is wrong (even though it works with the remaining code as it
stands), I'm afraid.

The right pattern is to `return 0' after
libxl__wait_device_connection, because libxl__wait_device_connection
will always make a callback.

Also the doc comment for this function should make it clear which of
the fields in aodev must be filled in by the caller.  After you do
that you should make sure that the call site obeys the rules.  (This
is not something I have checked right now because the rules aren't
stated.)

Both of these observations apply to the remove path as well.

> +/* AO function to remove a usb controller.
> + *
> + * Generally, it does:
> + * 1) check if the usb controller exists or not
> + * 2) remove all usb devices under controller
> + * 3) remove usb controller information from xenstore
> + */
> +void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
> +                                           libxl__ao_device *aodev)

What happens if this functionality is running to try to remove the
controller, at the same time as another process is trying to remove a
device from the controller, or (worse) add a device to the
controller ?  Obviously I don't expect 100% successful outcomes, but I
want to know that the severity of the consequences is limited.

> +libxl_device_usbctrl *
> +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
> +{

This function should return an rc, and the list should come in an out
parameter.

> +    dir = libxl__xs_directory(gc, XBT_NULL, path, &ndirs);

The variables `dir' and `ndirs' should be `entry' and `nentries'.

> +    if (dir && ndirs) {
> +        usbctrls = libxl__zalloc(NOGC, sizeof(*usbctrls) * ndirs);
> +        libxl_device_usbctrl *usbctrl;
> +        libxl_device_usbctrl *end = usbctrls + ndirs;
> +        for (usbctrl = usbctrls; usbctrl < end; usbctrl++, dir++, (*num)++) {

This line would be better wrapped at the semicolons, IMO.

> +            const char *tmp, *be_path;
> +            const char *fe_path = GCSPRINTF("%s/%s", path, *dir);
> +
> +            libxl_device_usbctrl_init(usbctrl);
> +            usbctrl->devid = atoi(*dir);
> +
> +            tmp = READ_FRONTEND(gc, "backend-id");
> +            if (!tmp) goto outerr;
> +            usbctrl->backend_domid = atoi(tmp);
> +
> +            tmp = READ_BACKEND(gc, "usb-ver");
> +            if (!tmp) goto outerr;
> +            usbctrl->version = atoi(tmp);
> +
> +            tmp = READ_BACKEND(gc, "num-ports");
> +            if (!tmp) goto outerr;
> +            usbctrl->ports = atoi(tmp);

There are 4 nearly identical stanzas here.  I think a more
comprehensive would be helpful.  Maybe a global macro READ_SUBPATH_INT
along the lines of my READ_SUBPATH, above, would be useful, and then:

               usbctrl->ports = READ_SUBPATH_INT(be_path, "num-ports");

and there would be no need for any open-coded error handling.

> +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                                libxl_device_usbctrl *usbctrl,
> +                                libxl_usbctrlinfo *usbctrlinfo)
...
> +    tmp = READ_FRONTEND(gc, "backend-id");
> +    usbctrlinfo->backend_id = tmp ? strtoul(tmp, NULL, 10) : -1;

Under what circumstances can these be validly missing ?  I'm not very
happy with the idea of filling in the struct with -1.

> +int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
> +                                  uint32_t domid,
> +                                  int devid,
> +                                  libxl_device_usbctrl *usbctrl)
> +{
> +    GC_INIT(ctx);
> +    libxl_device_usbctrl *usbctrls;
> +    int nb = 0;
> +    int i, rc = -1;

Initialising rc like this is a bad idea.  Instead, it should be set
explicitly on each error path.  That alllows the compiler to spot
error paths that do not set rc.  Also rc=-1 is not permitted because
rc ought to be a libxl error code.  You probably want ERROR_NOTFOUND.

> +static char *usb_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
> +{
> +        filename = GCSPRINTF(SYSFS_USB_DEV"/%s/devnum", de->d_name);
> +        if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +            sscanf(buf, "%d", &devnum);

If the file contents are invalid, devnum will be left uninitialised.
Why using sscanf rather than atoi (if you don't want to check for
unexpected data) or strtoul (if you do) ?  (Same thing later in
usb_busaddr_from_busid.)

> +        if (bus == busnum && addr == devnum) {
> +            busid = libxl__strdup(NOGC, de->d_name);
> +            break;

This allocates non-gc'd memory but (a) this is not documented contrary
to the rules in libxl_internal.h and (b) some of its callers do not
want non-gc'd memory.

> +static void usb_busaddr_from_busid(libxl__gc *gc, char *busid,
> +                                   uint8_t *bus, uint8_t *addr)

Lack of const-correctness: busid should be const char *.

> +    assert(busid);

This is not necessary.  Please drop it.  If it's NULL, we'll crash in
snprintf soon enough.


I think that many of my comments can be generalised to the rest of
this patch.  I think it would be better for me to stop reading now and
await a new version.  (I'm finding it difficult to see the overall
structure of the code, and to remember the things I'm trying to look
out for along with the things I'm trying to disregard because I have
already mentioned them.)

Specifically, can you please make sure to take every one of my
comments above, and treat it as relevant everywhere that it (or an
analagous comment) is applicable in your code.

Thanks,
Ian.

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