>>> On 8/11/2015 at 07:27 PM, in message
<20150811112702.gf7...@zion.uk.xensource.com>, Wei Liu <wei.l...@citrix.com>
wrote:
> On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> > 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
> >
> > Signed-off-by: Chunyan Liu <cy...@suse.com>
> > Signed-off-by: Simon Cao <caobosi...@gmail.com>
> >
> > ---
> > changes:
> > - Address George's comments:
> > * Update libxl_device_usb_getinfo to read ctrl/port only and
> > get other information.
> > * Update backend path according to xenstore frontend 'xxx/backend'
> > entry instead of using TOOLSTACK_DOMID.
> > * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
> > * Add USB 'devtype' union, currently only includes "hostdev"
> >
>
> I will leave this to Ian and George since they had strong opinions on
> this.
>
> I only skimmed this patch. Some comments below.
>
> [...]
> > +
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl_usbinfo *usbinfo);
> > +
> > /* Network Interfaces */
> > int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic
> *nic,
> > const libxl_asyncop_how *ao_how)
> > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > index bee5ed5..935f25b 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,
> libxl__devices_remove_state *drs)
> > aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
> > aodev->dev = dev;
> > aodev->force = drs->force;
> > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> > + libxl__initiate_device_usbctrl_remove(egc, aodev);
> > + continue;
> > + }
>
> Is there a risk that this races with individual device removal? I think
> you get away with it because removal of individual device is idempotent?
You mean races with other device removal (like 'vbd') ? Yes, it is idempotent.
Only for 'vusb' (corresponding to USB controller), before removing USB
controller
it will first removing all USB devices under it.
>
> > libxl__initiate_device_remove(egc, aodev);
> > }
> > }
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index f98f089..5be3b3a 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc,
> >
> uint32_t domid,
> > libxl_device_vtpm *vtpm,
> > libxl__ao_device *aodev);
> >
> > +_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_usbctrl *usbctrl,
> > + libxl__ao_device *aodev);
> > +
> > +_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl__ao_device *aodev);
> > +
> > /* Internal function to connect a vkb device */
> > _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
> > libxl_device_vkb *vkb);
> > @@ -2585,6 +2593,13 @@ _hidden void
> libxl__wait_device_connection(libxl__egc*,
> > _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> > libxl__ao_device *aodev);
> >
> > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> [...]
> > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl__ao_device *aodev)
> > +{
> > + STATE_AO_GC(aodev->ao);
> > + int rc = -1;
> > + char *busid = NULL;
> > +
> > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> > +
> > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > + usb->u.hostdev.hostaddr);
> > + if (!busid) {
> > + LOG(ERROR, "USB device doesn't exist in sysfs");
> > + goto out;
> > + }
> > +
> > + if (!is_usb_assignable(gc, usb)) {
> > + LOG(ERROR, "USB device is not assignable.");
> > + goto out;
> > + }
> > +
> > + /* check usb device is already assigned */
> > + if (is_usb_assigned(gc, usb)) {
> > + LOG(ERROR, "USB device is already attached to a domain.");
> > + goto out;
> > + }
> > +
> > + rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
> > + if (rc) goto out;
> > +
> > + rc = libxl__device_usb_add_xenstore(gc, domid, usb,
> > aodev->update_json);
> > + if (rc) goto out;
> > +
> > + rc = usbback_dev_assign(gc, usb);
> > + if (rc) {
> > + libxl__device_usb_remove_xenstore(gc, domid, usb);
> > + goto out;
> > + }
> > +
> > + libxl__ao_complete(egc, ao, 0);
> > + rc = 0;
> > +
> > +out:
>
> You forget to complete ao in failure path.
It will complete ao in aodev->callback(egc, aodev) in "out:" section, here:
if (rc) aodev->callback(egc, aodev);
Thanks,
Chunyan
>
> But I'm not very familiar with the AO machinery, I will let Ian comment
> on this.
>
> Wei.
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
