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 15:04, George Dunlap wrote:
> 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.

I guess using both (PVUSB + Qemu) for HVM, and let the guest decide
won't work, right?

Since libxl doesn't support PVUSB right now you could always name this
libxl_device_usb_add and sort the problem out once we have PVUSB
support. IMHO I would really prefer to only have one public function to
add USB devices if possible.

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

You could pass the same id that you are passing as an argument inside
libxl_device_usb id field.