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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, May 25, 2017 at 03:17:29PM +0300, Oleksandr Grytsov wrote:
> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
> 

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

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.

> 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?

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