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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 15.02.22 16:41, Jason Andryuk wrote:
> On Fri, Dec 10, 2021 at 7:35 AM Oleksandr Andrushchenko
> <andr2000@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 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