Re: [PATCH v2 2/4] tools: allow vchan XenStore paths more then 64 bytes long

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

Hi Dmitry,

On Wed, Jul 13, 2022 at 06:03:09PM +0300, dmitry.semenets@xxxxxxxxx wrote:
> diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
> index 9195bd3b98..38658f30af 100644
> --- a/tools/libs/vchan/init.c
> +++ b/tools/libs/vchan/init.c
> @@ -259,6 +259,12 @@ static int init_xs_srv(struct libxenvchan *ctrl, int 
> domain, const char* xs_base
>       if (!ctrl->xs_path)
>               return -1; 
>  
> +     buf = malloc(XENSTORE_ABS_PATH_MAX);
> +     if (!buf) {
> +             free(ctrl);
> +             return 0;

I don't understand what you are trying to achieve here. If we can't
allocate `buf`, we should return an error, right?
Also, `ctrl` isn't allocated in this function but by the caller, so I
don't think we need to free it here. Also, if it's free here, the caller
is going to continue to use the pointer, after free.

> +     }
> +
>       xs = xs_open(0);
>       if (!xs)
>               goto fail;
> @@ -419,13 +426,20 @@ struct libxenvchan *libxenvchan_client_init(struct 
> xentoollog_logger *logger,
>  {
>       struct libxenvchan *ctrl = malloc(sizeof(struct libxenvchan));
>       struct xs_handle *xs = NULL;
> -     char buf[64];
> +     char *buf;
>       char *ref;
>       int ring_ref;
>       unsigned int len;
>  
>       if (!ctrl)
>               return 0;
> +
> +     buf = malloc(XENSTORE_ABS_PATH_MAX);
> +     if (!buf) {
> +             free(ctrl);
> +             return 0;

Nit: could you write NULL instead of 0 here? It would makes it much
easier to understand that we return a pointer.


Thanks,

-- 
Anthony PERARD