[v1] tools: remove xenstore entries on vchan server closure

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

Posted by Oleksandr Andrushchenko 2 years, 4 months ago

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 */
-- 
2.25.1

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

Posted by Oleksandr Andrushchenko 2 years, 2 months ago

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 */

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

Posted by Jason Andryuk 2 years, 2 months ago

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

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

Posted by Oleksandr Andrushchenko 2 years, 2 months ago


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