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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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

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