Re: [PATCH v3 16/17] tools/xenstore: let check_store() check the accounting data

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 18/01/2023 06:23, Juergen Gross wrote:
> On 17.01.23 23:36, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 17/01/2023 09:11, Juergen Gross wrote:
> > > Today check_store() is only testing the correctness of the node tree.
> > >
> > > Add verification of the accounting data (number of nodes)  and correct
> > NIT: one too many space before 'and'.
> >
> > > the data if it is wrong.
> > >
> > > Do the initial check_store() call only after Xenstore entries of a
> > > live update have been read.
> > Can you clarify whether this is needed for the rest of the patch, or 
> > simply a nice thing to have in general?
> I'll add: "This is wanted to make sure the accounting data is correct after
> a live update."
Fine with me.

> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > >   tools/xenstore/xenstored_core.c   | 62 ++++++++++++++++------
> > >   tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++
> > >   tools/xenstore/xenstored_domain.h |  4 ++
> > >   3 files changed, 137 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/xenstore/xenstored_core.c 
> > > b/tools/xenstore/xenstored_core.c
> > > index 3099077a86..e201f14053 100644
> > > --- a/tools/xenstore/xenstored_core.c
> > > +++ b/tools/xenstore/xenstored_core.c
> > > @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update)
> > >           manual_node("@introduceDomain", NULL);
> > >           domain_nbentry_fix(dom0_domid, 5, true);
> > >       }
> > > -
> > > -    check_store();
> > >   }
> > >   static unsigned int hash_from_key_fn(void *k)
> > > @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, 
> > > const char *str)
> > >    * As we go, we record each node in the given reachable hashtable.  
> > > These
> > >    * entries will be used later in clean_store.
> > >    */
> > > +
> > > +struct check_store_data {
> > > +    struct hashtable *reachable;
> > > +    struct hashtable *domains;
> > > +};
> > > +
> > >   static int check_store_step(const void *ctx, struct connection *conn,
> > >                   struct node *node, void *arg)
> > >   {
> > > -    struct hashtable *reachable = arg;
> > > +    struct check_store_data *data = arg;
> > > -    if (hashtable_search(reachable, (void *)node->name)) {
> > > +    if (hashtable_search(data->reachable, (void *)node->name)) {
> > IIUC the cast is only necessary because hashtable_search() expects a 
> > non-const value. I can't think for a reason for the key to be 
> > non-const. So I will look to send a follow-up patch.
> It is possible, but nasty, due to talloc_free() not taking a const pointer.
I am not sure I understand your reasoning. Looking at 
hashtable_search(), I don't see a call to talloc_free().
Anyway, this is not directly related to this patch. So I will have a 
look separately.
Cheers,

--
Julien Grall