Re: [PATCH v2 14/18] tools/xenstore: move copying of node data out of db_fetch()

[Juergen Gross <jgross@xxxxxxxx>]

On 18.07.23 23:10, Julien Grall wrote:
> Hi Juergen,
>
> On 10/07/2023 07:59, Juergen Gross wrote:
> > Today the node data is copied in db_fetch() on each data base read in
> > order to avoid accidental data base modifications when working on a
> > node.
> >
> > read_node() is the only caller of db_fetch() which isn't freeing the
> > returned data area immediately after using it. The other callers don't
> > modify the returned data, so they don't need the data to be copied.
> This reads as the return value of db_fetch() should be const. This will at least 
> make more obvious to the caller that the value is not supposed to be modified.
This will add some more code churn. In the end I expect this to be much
more sane, though (e.g. talloc_free() taking a const pointer).

I'll look into that.

> > Move copying of the data into read_node(), resulting in a speedup of
> > the other callers due to no memory allocation and no copying being
> > needed anymore.
> >
> > As db_fetch() can't return any error other than ENOENT now, error
> > handling for the callers can be simplified.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - new patch
> > ---
> >   tools/xenstore/xenstored_core.c        | 41 ++++++++++----------------
> >   tools/xenstore/xenstored_transaction.c |  3 --
> >   2 files changed, 16 insertions(+), 28 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index 11da470b20..d5c9054fe9 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -557,8 +557,7 @@ static void initialize_fds(int *p_sock_pollfd_idx, int 
> > *ptimeout)
> >   struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size)
> >   {
> > -    const struct xs_tdb_record_hdr *hdr;
> > -    struct xs_tdb_record_hdr *p;
> > +    struct xs_tdb_record_hdr *hdr;
> >       hdr = hashtable_search(nodes, db_name);
> >       if (!hdr) {
> > @@ -569,18 +568,9 @@ struct xs_tdb_record_hdr *db_fetch(const char *db_name, 
> > size_t *size)
> >       *size = sizeof(*hdr) + hdr->num_perms * sizeof(hdr->perms[0]) +
> >           hdr->datalen + hdr->childlen;
> > -    p = talloc_size(NULL, *size);
> > -    if (!p) {
> > -        errno = ENOMEM;
> > -        return NULL;
> > -    }
> > -
> >       trace_tdb("read %s size %zu\n", db_name, *size + strlen(db_name));
> > -    /* Return a copy, avoiding a potential modification in the DB. */
> > -    memcpy(p, hdr, *size);
> > -
> > -    return p;
> > +    return hdr;
> >   }
> >   static void get_acc_data(const char *name, struct node_account_data *acc)
> > @@ -597,7 +587,6 @@ static void get_acc_data(const char *name, struct 
> > node_account_data *acc)
> >               acc->memory = size;
> >               acc->domid = hdr->perms[0].id;
> >           }
> > -        talloc_free(hdr);
> >       }
> >   }
> > @@ -731,30 +720,32 @@ struct node *read_node(struct connection *conn, const 
> > void *ctx,
> >       hdr = db_fetch(db_name, &size);
> >       if (hdr == NULL) {
> > -        if (errno == ENOENT) {
> > -            node->generation = NO_GENERATION;
> > -            err = access_node(conn, node, NODE_ACCESS_READ, NULL);
> > -            errno = err ? : ENOENT;
> > -        } else {
> > -            log("DB error on read: %s", strerror(errno));
> > -            errno = EIO;
> > -        }
> > +        node->generation = NO_GENERATION;
> > +        err = access_node(conn, node, NODE_ACCESS_READ, NULL);
> > +        errno = err ? : ENOENT;
> >           goto error;
> >       }
> >       node->parent = NULL;
> > -    talloc_steal(node, hdr);
> >       /* Datalen, childlen, number of permissions */
> >       node->generation = hdr->generation;
> >       node->perms.num = hdr->num_perms;
> >       node->datalen = hdr->datalen;
> >       node->childlen = hdr->childlen;
> > +    node->acc.domid = hdr->perms[0].id;
> > +    node->acc.memory = size;
> > +
> > +    /* Copy node data to new memory area, starting with permissions. */
> > +    size -= sizeof(*hdr);
> > +    node->perms.p = talloc_size(node, size);
> > +    if (node->perms.p == NULL) {
> > +        errno = ENOMEM;
> > +        goto error;
> > +    }
> > +    memcpy(node->perms.p, hdr->perms, size);
> >       /* Permissions are struct xs_permissions. */
> Is this comment still relevant?
I was on the edge, too. With you asking that question I'm happy to remove
the comment.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature