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

[Juergen Gross <jgross@xxxxxxxx>]

On 09.07.22 12:10, dmitry.semenets@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 d6010b145d..30cc73cf97 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 c8510e6ce9..c6b8674ef5 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
Please don't introduce new usages of the C++ comment style.

> +       ctrl->xs_path = strdup(xs_base);
Return an error in case of strdup() failure?

> +
>         xs = xs_open(0);
>         if (!xs)
>                 goto fail;
> @@ -298,6 +304,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);
> +       if (!xs)
> +               goto fail;
> +
> +       xs_rm(xs, XBT_NULL, ctrl->xs_path);
In this simple case I'd prefer

if (xs)
    xs_rm(xs, XBT_NULL, ctrl->xs_path);

> +
> +fail:
> +       free(ctrl->xs_path);
No xs_close()?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature