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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 17/04/13 10:54, Ian Campbell wrote:
> On Wed, 2013-04-17 at 10:36 +0100, George Dunlap wrote:
> > > > +/*
> > > > + * Just use plain numbers for now.  Replace these with strings at some point.
> > > > + */
> > > > +#define protocol_to_str(gc, t) libxl__sprintf((gc), "%d", (t))
> > > > +#define str_to_protocol(s) atoi((s))
> > > > +#define type_to_str(gc, t) libxl__sprintf((gc), "%d", (t))
> > > GCSPRINTF, although this macros are so simple that you might also
> > > replace them entirely with GCSPRINTF (altough it might make the code
> > > harder to understand).
> > I might do that -- my original plan had actually been to actually print
> > "pv" for LIBXL_DEVICE_USB_BACKEND_PV, "dm" for _DEVICEMODEL, and so on.
> > Doing all the extra parsing was more work than I wanted to try to get
> > done for the code freeze, but I left the functions so I could easily
> > replaced them later.
> These symbols should be coming from the IDL, in which case it will have
> provided the parsing functions already, please use them instead of
> inventing your own.
>
> e.g. The libxl_domain_type enum in the idl generates
> libxl_domain_type_from_string and libxl_domain_type_to_string (and
> _to_json if that floats your boat). These implement the mapping:
>         LIBXL_DOMAIN_TYPE_INVALID <=> "invalid"
>         LIBXL_DOMAIN_TYPE_HVM     <=> "hvm"
>         LIBXL_DOMAIN_TYPE_PV      <=> "pv"
> (the from function is case insensitive)
>
> "Replace these with strings at some point.", is that an API change,
> either for the libxl user or the driver side?
This is just an internal thing for libxl; it doesn't persist across reboots.

Let me take a look at the functions you mentioned -- if they're really 
easy I'll just drop them in.  Otherwise, the numeric values will be just 
fine, even if for some reason they end up being permanent.
> > > > +    switch (usbdev->protocol) {
> > > > +    case LIBXL_USB_PROTOCOL_DEVICEMODEL:
> > > > +        switch (libxl__device_model_version_running(gc, domid)) {
> > > > +        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> > > > +            LOG(ERROR, "usb-remove not yet implemented for qemu-traditional");
> > > > +            return ERROR_INVAL;
> > > > +        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> > > > +            if ( (rc = libxl__qmp_usb_remove(gc, domid, usbdev)) < 0 )
> > > Spaces
> > How important are the spaces?  Most of the time I think it makes it
> > easier to read, in this case particularly so.
> Everyone has their own opinions about these thing but the main purpose
> of having a Coding Style is consistency and libxl's style is to not
> include those spaces.
>
> If you want to make it easier to read I would suggest removing the
> assignment from the condition.
Ack.

 -George

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