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

[Julien Grall <julien@xen.org>]

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@suse.com>
>>> ---
>>>   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