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

[Juergen Gross <jgross@xxxxxxxx>]

On 13.12.22 10:35, Julien Grall wrote:
> 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.
Patch 2 of the 2nd series does that already.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature