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

[Dmytro Semenets <dmitry.semenets@xxxxxxxxx>]

пн, 11 июл. 2022 г. в 10:09, 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.
Most comments in this file are C++ style since libvchan introduced
>
> > +     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