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

[Juergen Gross <jgross@xxxxxxxx>]

On 18.01.23 10:35, Julien Grall wrote:
> 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().
Oh, I thought you were referring to the key in general.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature