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

[Juergen Gross <jgross@xxxxxxxx>]

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."

> > 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.

> > +
> > +void domain_check_acc_add(const struct node *node, struct hashtable *domains)
> > +{
> > +    struct domain_acc *dom;
> > +    unsigned int domid;
> > +
> > +    domid = node->perms.p[0].id;
> This could be replaced with get_node_owner().
Indeed.

> > +    dom = hashtable_search(domains, &domid);
> > +    if (!dom)
> > +        log("Node %s owned by unknown domain %u", node->name, domid);
> > +    else
> > +        dom->nodes++;
> > +}
> > +
> > +static int domain_check_acc_sub(const void *k, void *v, void *arg)
> I find the suffix 'sub' misleading because we have a function with a very 
> similar name (instead suffixed 'sub'). Yet, AFAICT, it is not meant to substract.
> So I would prefix with '_cb' instead.
Fine with me.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature