From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 1/2] libxl: Introduce functions to add and remove host USB devices to an HVM guest Date: Tue, 19 Mar 2013 14:04:40 +0000 Message-ID: <51487078.5000608@eu.citrix.com> References: <1363694970-17267-1-git-send-email-george.dunlap@eu.citrix.com> <514864C2.8060603@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <514864C2.8060603@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: Ian Jackson , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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