Re: [PATCH v2] tools: remove xenstore entries on vchan server closure

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

Hi Oleksandr,

On Wed, Feb 16, 2022 at 08:33:25AM +0200, Oleksandr Andrushchenko wrote:
> diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
> index c8510e6ce98a..ae9a6b579753 100644
> --- a/tools/libs/vchan/init.c
> +++ b/tools/libs/vchan/init.c
> @@ -251,6 +253,12 @@ static int init_xs_srv(struct libxenvchan *ctrl, int 
> domain, const char* xs_base
>       char ref[16];
>       char* domid_str = NULL;
>       xs_transaction_t xs_trans = XBT_NULL;
> +
> +     // store the base path so we can clean up on server closure
> +     ctrl->xs_path = strdup(xs_base);

You could do this in libxenvchan_server_init(), this might make it
easier to avoid leaking the path.

You need to initialise ctrl->xs_path in libxenvchan_server_init() in
any case before libxenvchan_close() is called.

I think you need to initialise ctrl->xs_path to NULL in
libxenvchan_client_init() as well even if it's not going to be used, to
avoid issue later in case the field start to be used.


> @@ -298,6 +306,23 @@ retry_transaction:
>       return ret;
>  }
>  
> +void close_xs_srv(struct libxenvchan *ctrl)
> +{
> +     struct xs_handle *xs;
> +
> +     if (!ctrl->xs_path)
> +             return;
> +
> +     xs = xs_open(0);

There is missing a xs_close() call.

> +     if (!xs)
> +             goto fail;
> +
> +     xs_rm(xs, XBT_NULL, ctrl->xs_path);
> +
> +fail:
> +     free(ctrl->xs_path);
> +}
> +
>  static int min_order(size_t siz)
>  {
>       int rv = PAGE_SHIFT;
> diff --git a/tools/libs/vchan/io.c b/tools/libs/vchan/io.c
> index da303fbc01ca..1f201ad554f2 100644
> --- a/tools/libs/vchan/io.c
> +++ b/tools/libs/vchan/io.c
> @@ -40,6 +40,8 @@
>  #include <xenctrl.h>
>  #include <libxenvchan.h>
>  
> +#include "vchan.h"
> +
>  #ifndef PAGE_SHIFT
>  #define PAGE_SHIFT 12
>  #endif
> @@ -384,5 +386,7 @@ void libxenvchan_close(struct libxenvchan *ctrl)
>               if (ctrl->gnttab)
>                       xengnttab_close(ctrl->gnttab);
>       }
> +     if (ctrl->is_server)
> +             close_xs_srv(ctrl);

Since init_xs_srv() is one of the last step of the initialisation of
*ctrl, I think close_xs_srv() should be one of the first step in
libxenvchan_close().

Thanks,

-- 
Anthony PERARD