| Message ID | 20211210123519.595163-1-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools: remove xenstore entries on vchan server closure | expand |
Anthony, could you please take a look? Thank you in advance, Oleksandr On 10.12.21 14:35, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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@epam.com> > --- > 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); > + > 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); > + > +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); > free(ctrl); > } > diff --git a/tools/libs/vchan/vchan.h b/tools/libs/vchan/vchan.h > new file mode 100644 > index 000000000000..621016ef42e5 > --- /dev/null > +++ b/tools/libs/vchan/vchan.h > @@ -0,0 +1,31 @@ > +/** > + * @file > + * @section AUTHORS > + * > + * Copyright (C) 2021 EPAM Systems Inc. > + * > + * @section LICENSE > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; If not, see <http://www.gnu.org/licenses/>. > + * > + * @section DESCRIPTION > + * > + * This file contains common libxenvchan declarations. > + */ > +#ifndef LIBVCHAN_H > +#define LIBVCHAN_H > + > +void close_xs_srv(struct libxenvchan *ctrl); > + > +#endif /* LIBVCHAN_H */
On Fri, Dec 10, 2021 at 7:35 AM Oleksandr Andrushchenko <andr2000@gmail.com> wrote: > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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@epam.com> > --- > 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
On 15.02.22 16:41, Jason Andryuk wrote: > On Fri, Dec 10, 2021 at 7:35 AM Oleksandr Andrushchenko > <andr2000@gmail.com> wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> 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@epam.com> >> --- >> 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. It does need a NULL check, thanks It is after writing code with all those allocations and garbage collector in the tools stack when allocations "don't fail" ;) But this is indeed not the case here and needs a proper check I'll wait for other comments and send v2 > > Regards, > Jason Thank you, Oleksandr
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); + 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); + +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); free(ctrl); } diff --git a/tools/libs/vchan/vchan.h b/tools/libs/vchan/vchan.h new file mode 100644 index 000000000000..621016ef42e5 --- /dev/null +++ b/tools/libs/vchan/vchan.h @@ -0,0 +1,31 @@ +/** + * @file + * @section AUTHORS + * + * Copyright (C) 2021 EPAM Systems Inc. + * + * @section LICENSE + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see <http://www.gnu.org/licenses/>. + * + * @section DESCRIPTION + * + * This file contains common libxenvchan declarations. + */ +#ifndef LIBVCHAN_H +#define LIBVCHAN_H + +void close_xs_srv(struct libxenvchan *ctrl); + +#endif /* LIBVCHAN_H */