Re: [PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting

[Juergen Gross <jgross@xxxxxxxx>]

On 03.05.23 10:02, Julien Grall wrote:
> Hi Juergen,
>
> On 03/05/2023 06:13, Juergen Gross wrote:
> > On 02.05.23 20:55, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 05/04/2023 08:03, Juergen Gross wrote:
> > > > Add the node accounting to the accounting information buffering in
> > > > order to avoid having to undo it in case of failure.
> > > >
> > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > ---
> > > >   tools/xenstore/xenstored_core.c   | 21 ++-------------------
> > > >   tools/xenstore/xenstored_domain.h |  4 ++--
> > > >   2 files changed, 4 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > > > index 84335f5f3d..92a40ccf3f 100644
> > > > --- a/tools/xenstore/xenstored_core.c
> > > > +++ b/tools/xenstore/xenstored_core.c
> > > > @@ -1452,7 +1452,6 @@ static void destroy_node_rm(struct connection *conn, 
> > > > struct node *node)
> > > >   static int destroy_node(struct connection *conn, struct node *node)
> > > >   {
> > > >       destroy_node_rm(conn, node);
> > > > -    domain_nbentry_dec(conn, get_node_owner(node));
> > > >       /*
> > > >        * It is not possible to easily revert the changes in a transaction.
> > > > @@ -1797,27 +1796,11 @@ static int do_set_perms(const void *ctx, struct 
> > > > connection *conn,
> > > >       old_perms = node->perms;
> > > >       domain_nbentry_dec(conn, get_node_owner(node));
> > > IIRC, we originally said that domain_nbentry_dec() could never fail in a 
> > > non-transaction case. But with your current rework, the function can now fail 
> > > because of an allocation failure.
> > How would that be possible to happen?
> >
> > domain_nbentry_dec() can only be called if a node is being owned by an already
> > known domain. So allocation is impossible to happen, as this would be a major
> > error in xenstored.
>  From my understanding, the nodes accounting will be temporary and then 
> committed at the end of the request.
> So we would call acc_add_changed_dom() which may require allocation to hold the 
> temporary accounting.
Ah, right, good catch!

Will add checking the return value and move the calls ahead of the tdb changes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature