From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH V7 3/7] libxl: add pvusb API Date: Fri, 2 Oct 2015 14:31:29 +0100 Message-ID: <1443792689.11707.117.camel@citrix.com> References: <1443147102-6471-1-git-send-email-cyliu@suse.com> <1443147102-6471-4-git-send-email-cyliu@suse.com> <560C2204.9030707@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <560C2204.9030707@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap , Chunyan Liu , xen-devel@lists.xen.org Cc: jgross@suse.com, wei.liu2@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, jfehlig@suse.com, Simon Cao List-Id: xen-devel@lists.xenproject.org On Wed, 2015-09-30 at 18:55 +0100, George Dunlap wrote: > > +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; > > + > > + usbctrls = libxl_device_usbctrl_list(ctx, domid, &nb); > > + if (!nb) goto out; > > + > > + for (i = 0; i < nb; i++) { > > + if (devid == usbctrls[i].devid) { > > + *usbctrl = usbctrls[i]; > > libxl maintainers: Is this kind of copying OK? > > The analog functions for vtpm and nic both take very different > approaches; both call libxl_device_[type]_init() and then fill in > individual elements (albeit in different ways). That depends on the memory lifecycle situation of usbctrls[i] and *usbctrl vis-a-vis the gc and when they are frred. Cut out of the context here is a + libxl_device_usbctrl_list_free(usbctrls, nb); which is going to free any of the pointers in usbctrls[i] which have been copied to usbctrl. So in this case no it is not ok. You can't avoid the libxl_device_usbctrl_list_free, since you don't want to leak all the other elements on the list, so copying seems to be the way to go. The IDL should have generated a copy function which can be used (by the vtpm one too, but it predates the IDL making such things I think). > > > +/* > > + * USB add > > + */ > > +static int do_usb_add(libxl__gc *gc, uint32_t domid, > > + libxl_device_usb *usbdev, > > + libxl__ao_device *aodev) > > +{ > > + libxl_ctx *ctx = CTX; > > + libxl_usbctrlinfo usbctrlinfo; > > + libxl_device_usbctrl usbctrl; > > + int rc; > > + > > + libxl_usbctrlinfo_init(&usbctrlinfo); > > + usbctrl.devid = usbdev->ctrl; > > + rc = libxl_device_usbctrl_getinfo(ctx, domid, &usbctrl, > > &usbctrlinfo); > > + if (rc) > > + goto out; > > + > > + switch (usbctrlinfo.type) { > > + case LIBXL_USBCTRL_TYPE_DEVICEMODEL: > > + LOG(ERROR, "Not supported"); > > + break; > > + 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); > > This assumes that the usb controller is dom0; but the interface > explicitly allows pvusb driver domains. > > I think it would be enough here to do this check if the usbctrl device > is dom0. We should then assume that a pvusb driver domain will be > configured with all usb devices assigned to usbback already. > > I assume that there's a feedback mechanisms for backends for situations > where the requested device can't be made, right? For example, if you > have a network driver domain and request a non-existent bridge? If so, > I think we can let the same mechanism worth for pvusb backends to say "I > don't have that device available". I think the b/e writes an error node in xenstore, which we already pickup iirc.