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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 03/19/2013 01:14 PM, Roger Pau Monne wrote:
> 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.

I had thought about something like that, but the basic problem is that 
HVM guests can use PVUSB as well. I don't think libxl is the right place 
to be deciding whether to use qemu or PVUSB if both are available.

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

OK -- I'll try to copy libxl_insert_cdrom(), since that seems to do the 
"fake async" thing.

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

The only way qmp allows you to remove a device is via id.  And at the 
moment there is no way via qmp to list USB devices to find out which 
ones might have an id.

I suppose if we allow the user to pass *dev though, then usb_del could 
either use dev->id (if it exists), or re-construct the default id and 
try to remove it if not.

  -George