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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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

Ian.


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