| Message ID | 20220216063325.1552217-1-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tools: remove xenstore entries on vchan server closure | expand |
On Wed, Feb 16, 2022 at 1:33 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> > > --- > 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
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,
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..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; @@ -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); + 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 */