Re: [PATCH v2 2/3] libxl: add new pvusb backend "qusb" provided by qemu

[Juergen Gross <jgross@suse.com>]

On 28/03/16 16:55, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 22, 2016 at 08:29:22AM +0100, Juergen Gross wrote:
>> Add a new pvusb backend type "qusb" which is provided by qemu. It can
>> be selected either by specifying the type directly in the configuration
>> or it is selected automatically by libxl in case there is no "usbback"
>> driver loaded.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  docs/man/xl.cfg.pod.5                |  11 +++-
>>  tools/libxl/libxl_device.c           |   3 +-
>>  tools/libxl/libxl_dm.c               |   8 +++
>>  tools/libxl/libxl_internal.h         |   1 +
>>  tools/libxl/libxl_pvusb.c            | 102 +++++++++++++++++++++++++++--------
>>  tools/libxl/libxl_types.idl          |   1 +
>>  tools/libxl/libxl_types_internal.idl |   1 +
>>  7 files changed, 101 insertions(+), 26 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index ec739cc..a4cc1b3 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -737,8 +737,15 @@ Possible B<KEY>s are:
>>  
>>  =item B<type=TYPE>
>>  
>> -Specifies the usb controller type.  Currently only 'pv' and 'auto'
>> -are supported.
>> +Specifies the usb controller type.
>> +
>> +"pv" denotes a kernel based pvusb backend.
>> +
>> +"qusb" specifies a qemu base backend for pvusb.
>> +
>> +"auto" (the default) determines whether a kernel based backend is installed.
>> +If this is the case, "pv" is selected, "qusb" will be selected if no kernel
>> +backend is currently available.
>>  
>>  =item B<version=VERSION>
>>  
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 4ced9b6..eba3087 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -680,7 +680,8 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
>>                  aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
>>                  aodev->dev = dev;
>>                  aodev->force = drs->force;
>> -                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB)
>> +                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
>> +                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
>>                      libxl__initiate_device_usbctrl_remove(egc, aodev);
>>                  else
>>                      libxl__initiate_device_generic_remove(egc, aodev);
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 897f3f9..361e584 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -2138,6 +2138,14 @@ int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config)
>>          }
>>      }
>>  
>> +    for (i = 0; i < d_config->num_usbctrls; i++) {
>> +       if (d_config->usbctrls[i].type == LIBXL_USBCTRL_TYPE_QUSB &&
>> +           d_config->usbctrls[i].backend_domid == domid) {
>> +            ret = 1;
>> +            goto out;
>> +        }
>> +    }
>> +
>>  out:
>>      return ret;
>>  }
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index fc7bdab..2db8b1b 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -487,6 +487,7 @@ typedef struct {
>>  #define QEMU_BACKEND(dev) (\
>>      (dev)->backend_kind == LIBXL__DEVICE_KIND_QDISK || \
>>      (dev)->backend_kind == LIBXL__DEVICE_KIND_VFB || \
>> +    (dev)->backend_kind == LIBXL__DEVICE_KIND_QUSB || \
>>      (dev)->backend_kind == LIBXL__DEVICE_KIND_VKBD)
>>  
>>  #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
>> diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
>> index 5f92628..7200ead 100644
>> --- a/tools/libxl/libxl_pvusb.c
>> +++ b/tools/libxl/libxl_pvusb.c
>> @@ -22,6 +22,21 @@
>>  
>>  #define USBHUB_CLASS_CODE 9
>>  
>> +static int usbback_is_loaded(libxl__gc *gc)
>> +{
>> +    int r;
>> +    struct stat st;
>> +
>> +    r = lstat(SYSFS_USBBACK_DRIVER, &st);
>> +
>> +    if (r == 0)
>> +        return 1;
>> +    if (r < 0 && errno == ENOENT)
>> +        return 0;
> 
> I believe the CODING STYLE in libxl asks for you to use { } for these
> ones.

No, it doesn't:

Quote from tools/libxl/CODING_STYLE:

5. Block structure

Every indented statement is braced apart from blocks that contain just
one statement.

> 
>> +    LOGE(ERROR, "Accessing %s", SYSFS_USBBACK_DRIVER);
> 
> Why is this an error?

What else? We can't determine whether the driver is loaded or not.
ENOENT is tested above, so it must be something weird.

> 
>> +    return -1;
>> +}
>> +
> 
> Otherwise looks OK to me.

Thanks,

Juergen


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