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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Feb 16, 2022 at 1:33 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>
>
> ---
> Since v1:
> - add NULL check after strdup
> ---
>  tools/include/libxenvchan.h |  5 +++++
>  tools/libs/vchan/init.c     | 25 +++++++++++++++++++++++++
>  tools/libs/vchan/io.c       |  4 ++++
>  tools/libs/vchan/vchan.h    | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 65 insertions(+)
>  create mode 100644 tools/libs/vchan/vchan.h

> 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
> @@ -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,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);
> +       if (!ctrl->xs_path)
> +               goto fail;
> +
>         xs = xs_open(0);
>         if (!xs)
>                 goto fail;

Hi, Oleksandr,

You now have multiple conditions goto fail, so I think you need to add
the below snippet to avoid leaking memory.

if (ret) {
    free(ctrl->xs_path)
    ctrl->xs_path = NULL;
}

It's actually okay with your patch since libxenvchan_server_init() does:

        if (init_xs_srv(ctrl, domain, xs_path, ring_ref))
                goto out;
        return ctrl;
out:
        libxenvchan_close(ctrl);
        return 0;

and libxenvchan_close() will free xs_path.  But it's a little more
robust to cleanup local to the failure.

Thanks,
Jason