On 15.02.22 16:41, Jason Andryuk wrote:
> On Fri, Dec 10, 2021 at 7:35 AM Oleksandr Andrushchenko
> <andr2...@gmail.com> wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@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_andrushche...@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
