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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Fri, Dec 10, 2021 at 7:35 AM Oleksandr Andrushchenko
<andr2000@xxxxxxxxx> wrote:
>
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
> vchan server creates XenStore entries to advertise its event channel and
> ring, but those are not removed after the server quits.
> Add additional cleanup step, so those are removed, so clients do not try
> to connect to a non-existing server.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
>  tools/include/libxenvchan.h |  5 +++++
>  tools/libs/vchan/init.c     | 23 +++++++++++++++++++++++
>  tools/libs/vchan/io.c       |  4 ++++
>  tools/libs/vchan/vchan.h    | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+)
>  create mode 100644 tools/libs/vchan/vchan.h
>
> diff --git a/tools/include/libxenvchan.h b/tools/include/libxenvchan.h
> index d6010b145df2..30cc73cf97e3 100644
> --- a/tools/include/libxenvchan.h
> +++ b/tools/include/libxenvchan.h
> @@ -86,6 +86,11 @@ struct libxenvchan {
>         int blocking:1;
>         /* communication rings */
>         struct libxenvchan_ring read, write;
> +       /**
> +        * Base xenstore path for storing ring/event data used by the server
> +        * during cleanup.
> +        * */
> +       char *xs_path;
>  };
>
>  /**
> diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
> index c8510e6ce98a..c6b8674ef541 100644
> --- a/tools/libs/vchan/init.c
> +++ b/tools/libs/vchan/init.c
> @@ -46,6 +46,8 @@
>  #include <xen/sys/gntdev.h>
>  #include <libxenvchan.h>
>
> +#include "vchan.h"
> +
>  #ifndef PAGE_SHIFT
>  #define PAGE_SHIFT 12
>  #endif
> @@ -251,6 +253,10 @@ 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 don't check for NULL here, but you do check for NULL in
close_xs_srv().  I guess it's okay, since it does the right thing.
But I think it would be more robust to check for NULL here.  Is there
a specific reason you wrote it this way?  Otherwise it looks good.

Regards,
Jason