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

[George Dunlap <george.dunlap@eu.citrix.com>]

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