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

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

>>> On 10/8/2015 at 10:41 PM, in message
<22038.32910.375958.407732@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
<Ian.Jackson@xxxxxxxxxxxxx> wrote: 
> 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. 

Sorry for replying late, the mail server were exchanged and had some
issues these days.

For the macro, I think maybe it's better to use local macros within function.
The macro itself doesn't do much work but a libxl__xs_read. Local macro
can have more flexibility, like can adding extra 'goto out' or error handling
or other repeated codes to the macro according to specific function.

> 
>
> 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  i
> 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.

It's OK to 'return 0' directly fater libxl__wait_device_connection since we 
don't
need to do cleanup work. Here the code follows the code format of
libxl__device_disk_add and libxl__device_nic_add, I think it doesn't harm.
 
>  
> 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.) 

The libxl__device_usbctrl_add follows the same rule as libxl__device_disk_add
and libxl__device_nic_add, call sites including DEFINE_DEVICE_ADD(usbctrl) and
domcreate time also follow the same rule as disk type and nic type. Do we need
to add doc comment to the parameters of this API separately?

>  
> 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.

I'm not quite clear about the whole lock mechanism in libxl code. But I think
here the risk is perhaps similar to one process doing domain destroy and at
the same time another process is trying to add a disk device. I didn't add extra
lock in the usbctrl add/remove and usb add/remove (should we??).

So, when one process removing usbctrl, another process is trying to add a 
device,
one case could be: the latter would fail to write xenstore if the former process
already removes xenstore entries and then report fail, but the former process
could succeed.
or could be: the latter process succeeds to write xenstore and assign usb device
to usbback and report success, but actually user cannot see the device, since
former process will remove everything under the controller soon.

For one process removing usbctrl, another removing usb device, it could be:
the latter one found the usb device doesn't exist any more and report error, but
the former one can succeed. or: the former one lists usb devices under the 
controller,
and trying to remove one by one; during that process, the latter removes some
usb device; then the former process will fail to remove that usb device again, 
and
then will report error.
 
>  
> > +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 
> is not something I have checked right now because the rules aren't 
> stated.)  
>  
> > +    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: 

Defining local macro within function may be better, can include more repeated 
codes
into the macro according to need.

>  
>                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.

In theory that should not happen.
 
>  
> > +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.

OK. Take it.
 
> 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.) 

No special reason, just to keep consistent to use the same code
everywhere getting sysfs info.

>
> > +        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. 

I'll have a look and add document if really need non-gc'd memory.

- Chunyan

>  
> > +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