Re: [PATCH V7 1/3] libxl: Add support for generic virtio device

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Dec 09, 2022 at 05:43:54AM +0530, Viresh Kumar wrote:
> On 08-12-22, 18:06, Anthony PERARD wrote:
> > Nit: Something like:
> >     const char check[] = "virtio,device";
> >     const size_t checkl = sizeof(check) - 1;
> >     ... strncmp(tmp, check, checkl)...
> >     (or just strncmp(tmp, check, sizeof(check)-1))
> > would avoid issue with both string "virtio,device" potentially been
> > different.
> 
> I think that is a generic problem with all the strings I am using. What about
> this diff over current patch ?
> 
> diff --git a/tools/libs/light/libxl_internal.h 
> b/tools/libs/light/libxl_internal.h
> index cdd155d925c1..a062fca0e2bb 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -166,6 +166,11 @@
>  /* Convert pfn to physical address space. */
>  #define pfn_to_paddr(x) ((uint64_t)(x) << XC_PAGE_SHIFT)
>  
> +/* Virtio device types */
> +#define VIRTIO_DEVICE_TYPE_GENERIC   "virtio,device"
> +#define VIRTIO_DEVICE_TYPE_GPIO      "virtio,device22"
> +#define VIRTIO_DEVICE_TYPE_I2C       "virtio,device29"

That a good idea.

>  /* logging */
>  _hidden void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int 
> errnoval,
>               const char *file /* may be 0 */, int line /* ignored if !file 
> */,
> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> index 64cec989c674..183d9c906e27 100644
> --- a/tools/libs/light/libxl_virtio.c
> +++ b/tools/libs/light/libxl_virtio.c
> @@ -62,7 +62,7 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const 
> char *libxl_path,
>                                         libxl_device_virtio *virtio)
>  {
>      const char *be_path, *tmp = NULL;
> -    int rc;
> +    int rc, len = sizeof(VIRTIO_DEVICE_TYPE_GENERIC) - 1;

That `len` variable is initialized quite far away from where it's used,
so ...
>  
>      virtio->devid = devid;
>  
> @@ -112,8 +110,8 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, 
> const char *libxl_path,
>      if (rc) goto out;
>  
>      if (tmp) {

... you could declare `len` here instead.

> -        if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {
> -            virtio->type = strdup(tmp);
> +        if (!strncmp(tmp, VIRTIO_DEVICE_TYPE_GENERIC, len)) {
> +            virtio->type = libxl__strdup(NOGC, tmp);
>          } else {
>              return ERROR_INVAL;
>          }
> 


Otherwise, those change looks fine.
Thanks,

-- 
Anthony PERARD