Re: [Xen-devel] [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

>>> +    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.

>>> +
>>> +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 *num)
>>> +{
>>> +    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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel