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

["Roger Pau Monné" <roger.pau@citrix.com>]

On 19/03/13 13:09, George Dunlap wrote:
> This uses the qmp functionality, and is thus only available for qemu-xen,
> not qemu-traditional.
> 
> Devices must be removed by "id", an identifying string, which must be
> specified when the device is created.  The caller can either pass one
> in to request; if none is passed in, then libxl will choose one and pass
> it back to the caller.
> 
> qemu will reject duplicate ids.  There is a small possibility that the
> libxl-chosen id my collide with a previously created one, in which
> case the add will fail.  It would be nice if the library could
> automatically modify it until it found a unique one, but at the moment
> qmp_run_command() doesn't return which error the command failed by, so
> the caller can't tell that the command failed because of a duplicate
> id.
> 
> Since it's additional work, and it's not clear that the situation can
> actually happen in practice, I'm considering it a "maybe do later".
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  tools/libxl/libxl.c          |   85 ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl.h          |   32 ++++++++++++++++
>  tools/libxl/libxl_internal.h |    3 ++
>  tools/libxl/libxl_qmp.c      |   45 ++++++++++++++++++++++
>  tools/libxl/libxl_types.idl  |    8 ++++
>  5 files changed, 173 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 572c2c6..34b648e 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2498,6 +2498,91 @@ out:
>      return AO_INPROGRESS;
>  }
> 
> +int libxl_hvm_host_usb_add(libxl_ctx *ctx, uint32_t domid,
> +                           libxl_device_host_usb *dev)

Would it make sense to just call this function libxl_device_usb_add and
fail if guest type is not HVM?

If we later add usb support to PV guests, I would prefer to avoid having
another libxl_pv_host_usb_add or libxl_pvh_host_usb_add.

Also you should add a "const libxl_asyncop_how *ao_how", even if this is
not an async op right now, we might want to make it async in the future.

> +{
> +    GC_INIT(ctx);
> +    int rc, dm_ver;
> +
> +    libxl_domain_type type = libxl__domain_type(gc, domid);
> +    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    if (type != LIBXL_DOMAIN_TYPE_HVM) {
> +        LOG(ERROR, "hvm-host-usb-add requires an HVM domain");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    if (libxl_get_stubdom_id(ctx, domid) != 0) {
> +        LOG(ERROR, "hvm-host-usb-add doesn't work for stub domains");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    dm_ver = libxl__device_model_version_running(gc, domid);
> +    if (dm_ver == -1) {
> +        LOG(ERROR, "cannot determine device model version");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> +        rc = libxl__qmp_host_usb_add(gc, domid, dev);
> +    } else {
> +        LOG(ERROR, "hvm-host-usb-add not yet implemented for qemu-traditional");
> +        rc = ERROR_FAIL;
> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_hvm_host_usb_del(libxl_ctx *ctx, uint32_t domid,
> +                           const char * id)

Same here, libxl_device_usb_remove will probably be a better name to
keep in sync with the current device functions, and it's also missing a
"const libxl_asyncop_how *ao_how".

Is it possible to pass a libxl_device_host_usb *dev instead of an id? So
that the function resembles to the other _remove/_destroy functions.