|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API
On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote:
>
>
> >>> On 8/11/2015 at 07:27 PM, in message
> <20150811112702.GF7460@xxxxxxxxxxxxxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>
> 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 <cyliu@xxxxxxxx>
> > > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
> > >
> > > ---
> > > 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.
>
No. What I mean is, the removal of usbctrl triggers removal of all
assigned usb devices. And then this function initiates removal of
assigned usb devices again. Is this a possible scenario?
> >
> > > 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);
>
I'm still confused by the way it is structured. If aodev->callback
completes the AO nonetheless, why don't you just call that
unconditionally?
Wei.
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |