Re: [Xen-devel] [PATCH v2 1/3] libxl: add PV display device driver interface

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Tue, Jun 20, 2017 at 4:54 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> On Thu, May 25, 2017 at 03:17:29PM +0300, Oleksandr Grytsov wrote:
>> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>

Hi Wei,

Thank you for your reply.

> I'm sorry, patch like this is impossible to review because: 1. there is
> no commit message 2. it is huge.

I will separate it to small ones and will add commit message.

> I can see it is adding a lot of hooks to the device handling framework.
> Please explain why they are needed. This sort of changes (refactoring
> and extending existing code) should also be in separate patches.

Hooks in the device handling framework is needed to avoid code duplication
on new PV device adding. There were two possibilities either use macro
or extend the device handling framework. See [1] and following conversation
for more details. The patches don't refactor existing code they extend
hooks to the device handling framework and add new functionality required
to add the display device driver.

Almost all libxl__device_xxxx_add functions are the same except Xen store
parameters. So, I've moved setting Xen store parameters  to
set_xenstore_config hook
and have created common function to add device (add hook).
Also I've created functions to libxl_device_xxxx_list and
libxl_device_xxxx_list_free.
which take device type as parameter.

>
>> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>> ---
>>  tools/libxl/Makefile                 |   2 +-
>>  tools/libxl/libxl.h                  |  21 ++
>>  tools/libxl/libxl_create.c           |   3 +
>>  tools/libxl/libxl_device.c           | 178 ++++++++++++++++-
>>  tools/libxl/libxl_internal.h         |  24 +++
>>  tools/libxl/libxl_types.idl          |  40 +++-
>>  tools/libxl/libxl_types_internal.idl |   1 +
>>  tools/libxl/libxl_usb.c              |   2 +
>>  tools/libxl/libxl_utils.h            |   4 +
>>  tools/libxl/libxl_vdispl.c           | 372 
>> +++++++++++++++++++++++++++++++++++
>>  10 files changed, 643 insertions(+), 4 deletions(-)
>>  create mode 100644 tools/libxl/libxl_vdispl.c
>>  };
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 5e96676..2954800 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -18,7 +18,7 @@
>>
>>  #include "libxl_internal.h"
>>
>> -static char *libxl__device_frontend_path(libxl__gc *gc, libxl__device 
>> *device)
>> +char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
>>  {
>>      char *dom_path = libxl__xs_get_dompath(gc, device->domid);
>>
>> @@ -1776,6 +1776,182 @@ out:
>>      return AO_CREATE_FAIL(rc);
>>  }
>>
>> +static int device_add_domain_config(libxl__gc *gc, uint32_t domid,
>> +                                    const struct libxl_device_type *dt,
>> +                                    void *type)
> [...]
>> +
>> +void libxl__device_add(libxl__egc *egc, uint32_t domid,
>> +                       const struct libxl_device_type *dt, void *type,
>> +                       libxl__ao_device *aodev)
> [...]
>> +
>> +void* libxl__device_list(const struct libxl_device_type *dt,
>> +                         libxl_ctx *ctx, uint32_t domid, int *num)
> [...]
>> +
>> +void libxl__device_list_free(const struct libxl_device_type *dt,
>> +                             void *list, int num)
>>
>
> I think existing code already provides these functionalities, right?

Right, but as I mentioned before there are almost same functions for
each device. These new functions are generic.

[1] http://marc.info/?l=xen-devel&m=149026463411873&w=2

-- 
Best Regards,
Oleksandr Grytsov.

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