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 01:39:27 -0600 Message-ID: <557AFD2F02000066000D4C7D@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21881.47707.526863.158586@mariner.uk.xensource.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 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 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? > 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 > >