Re: [Xen-devel] [PATCH v1 2/5] libxl: add vsnd list and info

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Tue, Feb 6, 2018 at 4:43 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:

On Wed, Nov 01, 2017 at 05:04:44PM +0200, Oleksandr Grytsov wrote:
> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>
> Add getting vsnd list amd info API
>
> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
> ---
>  tools/libxl/libxl.h         |  10 ++
>  tools/libxl/libxl_types.idl |  19 +++
>  tools/libxl/libxl_utils.h   |   3 +
>  tools/libxl/libxl_vsnd.c    | 375 +++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 404 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 7200d49..acb73ce 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1927,6 +1927,16 @@ int libxl_device_vsnd_destroy(libxl_ctx *ctx, uint32_t domid,
>                                const libxl_asyncop_how *ao_how)
>                                LIBXL_EXTERNAL_CALLERS_ONLY;
>
> +libxl_device_vsnd *libxl_device_vsnd_list(libxl_ctx *ctx,
> +                                          uint32_t domid, int *num)
> +                                          LIBXL_EXTERNAL_CALLERS_ONLY;
> +void libxl_device_vsnd_list_free(libxl_device_vsnd* list, int num)
> +                                 LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vsnd_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                              libxl_device_vsnd *vsnd,
> +                              libxl_vsndinfo *vsndlinfo)
> +                              LIBXL_EXTERNAL_CALLERS_ONLY;
> +
>  /* Keyboard */
>  int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb,
>                           const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index aa30196..553e724 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -988,6 +988,25 @@ libxl_vdisplinfo = Struct("vdisplinfo", [
>      ("connectors", Array(libxl_connectorinfo, "num_connectors"))
>      ], dir=DIR_OUT)
>
> +libxl_streaminfo = Struct("streaminfo", [
> +    ("req_evtch", integer),
> +    ("req_rref", integer)
> +    ])
> +
> +libxl_pcminfo = Struct("pcminfo", [
> +    ("streams", Array(libxl_streaminfo, "num_vsnd_streams"))
> +    ])
> +
> +libxl_vsndinfo = Struct("vsndinfo", [
> +    ("backend", string),
> +    ("backend_id", uint32),
> +    ("frontend", string),
> +    ("frontend_id", uint32),
> +    ("devid", libxl_devid),
> +    ("state", integer),
> +    ("pcms", Array(libxl_pcminfo, "num_vsnd_pcms"))
> +    ])
> +
>  # NUMA node characteristics: size and free are how much memory it has, and how
>  # much of it is free, respectively. dists is an array of distances from this
>  # node to each other node.
> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index 9e743dc..5455752 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
> @@ -82,6 +82,9 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, uint32_t domid,
>  int libxl_devid_to_device_vdispl(libxl_ctx *ctx, uint32_t domid,
>                                   int devid, libxl_device_vdispl *vdispl);
>
> +int libxl_devid_to_device_vsnd(libxl_ctx *ctx, uint32_t domid,
> +                               int devid, libxl_device_vsnd *vsnd);
> +
>  int libxl_ctrlport_to_device_usbdev(libxl_ctx *ctx, uint32_t domid,
>                                      int ctrl, int port,
>                                      libxl_device_usbdev *usbdev);
> diff --git a/tools/libxl/libxl_vsnd.c b/tools/libxl/libxl_vsnd.c
> index 99e4be3..35f1aed 100644
> --- a/tools/libxl/libxl_vsnd.c
> +++ b/tools/libxl/libxl_vsnd.c
> @@ -37,22 +37,247 @@ static int libxl__device_from_vsnd(libxl__gc *gc, uint32_t domid,
>     return 0;
>  }
>
> +static int libxl__sample_rates_from_string(libxl__gc *gc, const char *str,
> +                                           libxl_vsnd_params *params)
> +{
> +    char *tmp = libxl__strdup(gc, str);
> +
> +    params->num_sample_rates = 0;
> +    params->sample_rates = NULL;
> +
> +    char *p = strtok(tmp, " ,");
> +
> +    while (p != NULL) {
> +        params->sample_rates = realloc(params->sample_rates,
> +                                       sizeof(*params->sample_rates) *
> +                                       (params->num_sample_rates + 1));

This is problematic. You need to check if realloc returns NULL before
overwriting sample_rates.

It is also a bit expensive to realloc 1 element at a time. Is is
possible to know the size before hand? If not, then fine.

Please use libxl__realloc instead. We have quite a few wrappers in
libxl. In general please use them unless you have very compelling reason
not to.

There could be other places in your two series that I missed, please fix
them.

Wei.

Yes, I agree that it is bit expensive to make realloc for each element.

I will count num elements before. About using realloc instead of
libxl_realloc in this place. it is autogenerated structure and it will be

freed with appropriate API. Using libxl__realloc will cause double free

issue.

--

Best Regards,
Oleksandr Grytsov.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel