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

[Anthony PERARD <anthony.perard@citrix.com>]

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