From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chun Yan Liu" Subject: Re: [PATCH V4 3/7] libxl: add pvusb API Date: Fri, 12 Jun 2015 02:06:48 -0600 Message-ID: <557B039802000066000D4CCB@relay2.provo.novell.com> References: <1433906441-3280-1-git-send-email-cyliu@suse.com> <1433906441-3280-4-git-send-email-cyliu@suse.com> <21881.47707.526863.158586@mariner.uk.xensource.com> <557AFD2F02000066000D4C7D@relay2.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <557AFD2F02000066000D4C7D@relay2.provo.novell.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson , Chun Yan Liu Cc: george.dunlap@eu.citrix.com, Simon Cao , wei.liu2@citrix.com, ian.campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 6/12/2015 at 03:39 PM, in message <557AFD2F02000066000D4C7D@relay2.provo.novell.com>, "Chun Yan Liu" wrote: > >>>> On 6/12/2015 at 12:42 AM, in message > <21881.47707.526863.158586@mariner.uk.xensource.com>, Ian Jackson > wrote: > > Chunyan Liu writes ("[Xen-devel] [PATCH V4 3/7] libxl: add pvusb API"): > > > Add pvusb APIs, including: > > ... > > > > Thanks for your contribution. I'm afraid I haven't had time to > > completely finish my review this, but here's what I have: > > > > > --- /dev/null > > > +++ b/docs/misc/pvusb.txt > > > @@ -0,0 +1,243 @@ > > > +1. Overview > > > > It's good that you have provided documentation. But I think this > > document is a bit confused about its audience. > > > > Information about design choices should be removed from this user- and > > application-facing document, and put in comments in the code, or > > commit messages, I think. > > Thanks. Will update. > > > > > > > > +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid, > > > + libxl_device_usbctrl *usbctrl, > > > + libxl_usbctrlinfo *usbctrlinfo) > > > + LIBXL_EXTERNAL_CALLERS_ONLY; > > > > Why is this function marked LIBXL_EXTERNAL_CALLERS_ONLY ? > > Currently libxl itself won't call it. Exposed for toolstack usage. Will > remove that if it's not proper. > > > > > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > > > index ef7aa1d..89a9f07 100644 > > > --- a/tools/libxl/libxl_internal.h > > > +++ b/tools/libxl/libxl_internal.h > > > @@ -2439,6 +2439,18 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, > > > uint32_t domid, > > ... > > > +/* AO operation to connect a PVUSB controller. > > > + * Adding a PVUSB controller will add a 'vusb' device entry in xenstore, > > > + * and will wait for device connection. > > > > In this context I think "will wait for device connection" is > > misleading. What you mean is that the vusb is available for adding > > devices to, but won't have any yet. Nothing in libxl is "waiting". > > Here I mean libxl_wait_for_device_connection. Since it adds a new device > entry > to xenstore, it needs to wait for a moment for frontend/backend connection. > > > > > > diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c > > > new file mode 100644 > > > index 0000000..a6e1aa1 > > > --- /dev/null > > > +++ b/tools/libxl/libxl_pvusb.c > > > @@ -0,0 +1,1249 @@ > > ... > > > +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_domain_config_init(&d_config); > > > + libxl_device_usbctrl_init(&usbctrl_saved); > > > + libxl_device_usbctrl_copy(CTX, &usbctrl_saved, usbctrl); > > > > Wei will need to review the saved/live saved device info handling, and > > the json, etc. > > > > > +static int > > > +libxl_device_usbctrl_remove_common(libxl_ctx *ctx, uint32_t domid, > > > + libxl_device_usbctrl *usbctrl, > > > + const libxl_asyncop_how *ao_how, > > > + int force) > > > > As discussed, you mustn't call this within libxl. > I don't quite understand why. I guess it's the same as usb_add problem, > something related to embedded ao? > As in usb_add: > libxl_device_usb_add() > AO_CREATE(ctx, domid, ao_how) > libxl__device_usb_add() > libxl__device_usb_setdefault() > libxl_device_usbctrl_add_common() > AO_CREATE(ctx, domid, NULL) > if this is not allowed, what is the correct way? Saw the discussion thread and got it. Will update the codes. > > > If you need to, you > > need to break it out into an internal function > > (libxl__initiate_device_usbctrl_remove, maybe) which makes a callback > > when done. > > > > > +libxl_device_usbctrl * > > > +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num) > > > +{ > > > + GC_INIT(ctx); > > > + libxl_device_usbctrl *usbctrls = NULL; > > > + char *fe_path = NULL; > > > + char **dir = NULL; > > > + unsigned int ndirs = 0; > > > + > > > + *num = 0; > > > + > > > + fe_path = GCSPRINTF("%s/device/vusb", > > > + libxl__xs_get_dompath(gc, domid)); > > > + dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs); > > > + > > > + if (dir && ndirs) { > > > + usbctrls = malloc(sizeof(*usbctrls) * ndirs); > > > > Please use libxl__calloc with NOGC. > > Thanks. Missing this one. > > > > > > + libxl_device_usbctrl* usbctrl; > > > + libxl_device_usbctrl* end = usbctrls + ndirs; > > > + for (usbctrl = usbctrls; usbctrl < end; usbctrl++, dir++, > (*num)++) > > { > > > + char *tmp; > > > + const char *be_path = libxl__xs_read(gc, XBT_NULL, > > > + GCSPRINTF("%s/%s/backend", fe_path, > > *dir)); > > > + > > > + libxl_device_usbctrl_init(usbctrl); > > > + usbctrl->devid = atoi(*dir); > > > > This function (and the corresponding writing code) is quite formulaic. > > Perhaps a macro or something could be used. > > > > I would make a similar criticism of libxl_device_usbctrl_getinfo. > > > > > +/* check if USB device is already assigned to a domain */ > > > +static bool is_usb_assigned(libxl__gc *gc, libxl_device_usb *usb) > > > +{ > > > + libxl_device_usb *usbs; > > > + int rc, num; > > > + > > > + rc = libxl__device_usb_assigned_list(gc, &usbs, &num); > > > + if (rc) { > > > + LOG(ERROR, "Fail to get assigned usb list"); > > > + return true; > > > > I don't think this is proper error handling. You need to either > > encode the boolean return value in the error code, or have the boolean > > result be a reference parameter. > > Will improve that. > > Thanks, > Chunyan > > > > > > > Thanks, > > Ian. > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >