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

[Julien Grall <julien@xxxxxxx>]

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.
> 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?

> -       node->perms.p = hdr->perms;
> -       node->acc.domid = get_node_owner(node);
> -       node->acc.memory = size;
>         if (domain_adjust_node_perms(node))
>                 goto error;
>   
> diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
> index 5d7da82aad..e3e05a1d84 100644
> --- a/tools/xenstore/xenstored_transaction.c
> +++ b/tools/xenstore/xenstored_transaction.c
> @@ -365,13 +365,10 @@ static int finalize_transaction(struct connection *conn,
>                 if (i->check_gen) {
>                         hdr = db_fetch(i->node, &size);
>                         if (!hdr) {
> -                               if (errno != ENOENT)
> -                                       return errno;
>                                 gen = NO_GENERATION;
>                         } else {
>                                 gen = hdr->generation;
>                         }
> -                       talloc_free(hdr);
>                         if (i->generation != gen)
>                                 return EAGAIN;
>                 }
Cheers,

--
Julien Grall