Re: [PATCH v3 16/25] tools/xenstore: move copying of node data out of db_fetch()

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 24/07/2023 12:02, 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.
>
> 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.
>
> This allows to let db_fetch() return a pointer to const data.
>
> 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
> V3:
> - modify return type of db_fetch() to return a pointer to const
>    (Julien Grall)
> - drop stale comment (Julien Grall)
> - fix transaction handling
> ---
>   tools/xenstore/xenstored_core.c        | 45 +++++++++++---------------
>   tools/xenstore/xenstored_core.h        |  2 +-
>   tools/xenstore/xenstored_transaction.c | 23 +++++++++----
>   3 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 9f88914149..1f5f118f1c 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -555,10 +555,9 @@ 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 *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;
Can't 'hdr' stay const?

>   
>   	hdr = hashtable_search(nodes, db_name);
>         if (!hdr) {
[...]

> @@ -388,14 +385,26 @@ static int finalize_transaction(struct connection *conn,
>                 if (i->ta_node) {
>                         hdr = db_fetch(i->trans_name, &size);
>                         if (hdr) {
> +                               /*
> +                                * Delete transaction entry and write it as
> +                                * no-TA entry. As we only hold a reference
> +                                * to the data, increment its ref count, then
> +                                * delete it from the DB. Now we own it and can
> +                                * drop the const attribute for changing the
It is not great, but I understand this is another necessary evil. So I 
am ok with this cast-away const. It is also well documented.
> +                                * generation count.
> +                                */
>                                 enum write_node_mode mode;
> +                               struct xs_tdb_record_hdr *own;
>   
> -				hdr->generation = ++generation;
> +                               talloc_increase_ref_count(hdr);
> +                               db_delete(conn, i->trans_name, NULL);
> +
> +                               own = (struct xs_tdb_record_hdr *)hdr;
> +                               own->generation = ++generation;
>                                 mode = (i->generation == NO_GENERATION)
>                                        ? NODE_CREATE : NODE_MODIFY;
> -                               *is_corrupt |= db_write(conn, i->node, hdr,
> +                               *is_corrupt |= db_write(conn, i->node, own,
>                                                         size, NULL, mode, true);
> -                               db_delete(conn, i->trans_name, NULL);
>                         } else {
>                                 *is_corrupt = true;
>                         }
Cheers,

--
Julien Grall