From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest Date: Wed, 17 Apr 2013 11:05:24 +0100 Message-ID: <516E73E4.4010502@eu.citrix.com> References: <1365706317-5368-1-git-send-email-george.dunlap@eu.citrix.com> <516D91AE.4030600@citrix.com> <516E6D0F.4090606@eu.citrix.com> <516E6FD4.8070602@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <516E6FD4.8070602@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: Roger Pau Monne Cc: "sstanisi@cbnco.com" , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 17/04/13 10:48, Roger Pau Monne wrote: >>>> + if (!libxl_usb_path) { >>>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths"); >>>> + rc = ERROR_FAIL; >>>> + goto out; >>>> + } >>>> + >>>> noperm[0].id = 0; >>>> noperm[0].perms = XS_PERM_NONE; >>>> >>>> @@ -475,6 +482,9 @@ retry_transaction: >>>> xs_rm(ctx->xsh, t, libxl_path); >>>> libxl__xs_mkdir(gc, t, libxl_path, noperm, ARRAY_SIZE(noperm)); >>>> >>>> + xs_rm(ctx->xsh, t, libxl_usb_path); >>> You are missing the error check, there's also a helper for xs_rm, >>> libxl__xs_rm_checked. >> I'm just following suit with what all the other xs_rm's do here. I >> think it's actually expected that there will *not* be such a path, but >> that it's just cleaning up to be sure. > libxl__xs_rm_checked will not return an error if xs_rm errno is ENOENT, > it will only return an error if something really bad happened. OK -- well, if you look at that whole function, there are dozens of things removed and added with no checking. I think it's counter-productive to add checking in only one place -- it gives people the impression that this is something new and different, when in fact it's exactly the same as everything else. The alternate would be to add a patch to add error-checking to every single one. But I think at this point in the release cycle, that's too risky. It's a lot of code churn for no clear benefit, and there's the possibility we'll introduce a bug that will be discovered at the 11th hour (or in fact after the release). I can add cleaning up this function in my to-do list for the 4.4 dev cycle if you want. > >>>> + >>>> +out: >>>> + return rc; >>>> +} >>>> + >>>> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, >>>> + libxl_device_usb *usbdev, >>>> + const libxl_asyncop_how *ao_how) >>>> +{ >>>> + AO_CREATE(ctx, domid, ao_how); >>>> + int rc; >>>> + rc = libxl__device_usb_add(gc, domid, usbdev); >>>> + libxl__ao_complete(egc, ao, rc); >>>> + return AO_INPROGRESS; >>>> +} >>>> + >>>> +/* >>>> + * USB remove >>>> + */ >>>> +static int do_usb_remove(libxl__gc *gc, uint32_t domid, >>>> + libxl__device_usb *usbdev) >>>> +{ >>>> + int rc; >>>> + >>>> + switch (usbdev->protocol) { >>>> + case LIBXL_USB_PROTOCOL_DEVICEMODEL: >>>> + switch (libxl__device_model_version_running(gc, domid)) { >>>> + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: >>>> + LOG(ERROR, "usb-remove not yet implemented for qemu-traditional"); >>>> + return ERROR_INVAL; >>>> + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: >>>> + if ( (rc = libxl__qmp_usb_remove(gc, domid, usbdev)) < 0 ) >>> Spaces >> How important are the spaces? Most of the time I think it makes it >> easier to read, in this case particularly so. > It's in libxl CODING_STYLE, since you are already caching the return > value, why don't you split the line: > > rc = libxl__qmp_usb_remove(gc, domid, usbdev); > if (rc < 0) > ... > > This makes it even easier to read IMHO. > >>>> +out: >>>> + return rc; >>>> +} >>>> + >>>> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, >>>> + libxl_device_usb *usbdev, >>>> + const libxl_asyncop_how *ao_how) >>>> +{ >>>> + AO_CREATE(ctx, domid, ao_how); >>>> + int rc; >>>> + rc = libxl__device_usb_remove(gc, domid, usbdev); >>>> + libxl__ao_complete(egc, ao, rc); >>>> + return AO_INPROGRESS; >>>> +} >>>> + >>>> + >>>> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, >>>> + uint32_t domid, int *nu >>>> >>>> m) >>>> +{ >>>> + GC_INIT(ctx); >>>> + libxl__device_usb *devint; >>>> + libxl_device_usb *devext = NULL; >>>> + int n = 0, i, rc; >>>> + >>>> + rc = get_assigned_devices(gc, domid, &devint, &n); >>>> + if ( rc ) { >>>> + LOG(ERROR, "Could not get assigned devices"); >>>> + goto out; >>>> + } >>>> + >>>> + if(n == 0) >>>> + goto out; >>>> + >>>> + devext = calloc(n, sizeof(libxl_device_usb)); >>> libxl__calloc, also you seem to leak this allocation, which will be >>> solved by the use of libxl__calloc. >> This isn't a leak -- this is giving the list to an external caller, who >> is responsible to free the list. This follows suit with other >> libxl_device_.*_list() functions, which must free() the return value >> themselves. (See e.g., libxl_device_pci_list(), >> libxl_device_pci_assignable_list(), libxl_device_nic_list(), &c.) > OK, then you can use libxl__calloc with NOGC. Sure, I guess. :-) -George