Re: [PATCH 11/20] tools/xenstore: move changed domain handling

[Julien Grall <julien@xen.org>]

Hi Juergen,

On 13/12/2022 06:53, Juergen Gross wrote:
> On 01.12.22 22:58, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 01/11/2022 15:28, Juergen Gross wrote:
>>>   static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX 
>>> prod)
>>> @@ -492,8 +504,12 @@ static struct domain 
>>> *find_or_alloc_existing_domain(unsigned int domid)
>>>       xc_dominfo_t dominfo;
>>>       domain = find_domain_struct(domid);
>>> -    if (!domain && get_domain_info(domid, &dominfo))
>>> -        domain = alloc_domain(NULL, domid);
>>> +    if (!domain) {
>>> +        if (!get_domain_info(domid, &dominfo))
>>> +            errno = ENOENT;
>>> +        else
>>> +            domain = alloc_domain(NULL, domid);
>>> +    }
>>
>> I don't understand how this change is related to this commit.
> 
> It is directly related to the hunk below. Returning errno in
> acc_add_dom_nbentry() requires it to be set correctly in
> find_or_alloc_existing_domain().
> 
> I'll add a remark in the commit message.
> 
>>
>> [...]
>>
>>> +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int 
>>> val,
>>> +            unsigned int domid)
>>> +{
>>> +    struct changed_domain *cd;
>>> +
>>> +    cd = acc_get_changed_domain(ctx, head, domid);
>>> +    if (!cd)
>>> +        return errno;
>>> +
>>> +    cd->nbentry += val;
>>
>> As a future improvement, it would be worth considering to check for 
>> underflow/overflow.
> 
> Do you really think we need to make sure not to add/remove more than
> 2 billion nodes owned by a single domain?
No and that's not my point. If you look at domain_entry_fix() we have an 
assert() to check if the sum is still over 0.

This assert() was actually triggered a few times while testing the 
previous XSAs batch. So I think it would be worth to carry a similar 
check (maybe not an assert()) just in case we mess up with accounting in 
the future.

Cheers,

-- 
Julien Grall