Re: [PATCH v2 09/13] tools/xenstore: add TDB access trace support

[Julien Grall <julien@xxxxxxx>]

Hi,

On 20/01/2023 10:00, Juergen Gross wrote:
> Add a new trace switch "tdb" and the related trace calls.
>
> The "tdb" switch is off per default.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
With one remark (see below):

Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
>   tools/xenstore/xenstored_core.c        | 8 +++++++-
>   tools/xenstore/xenstored_core.h        | 6 ++++++
>   tools/xenstore/xenstored_transaction.c | 7 ++++++-
>   3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 558ef491b1..49e196e7ae 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -589,6 +589,8 @@ static void get_acc_data(TDB_DATA *key, struct 
> node_account_data *acc)
>                 if (old_data.dptr == NULL) {
>                         acc->memory = 0;
>                 } else {
> +                       trace_tdb("read %s size %zu\n", key->dptr,
> +                                 old_data.dsize + key->dsize);
>                         hdr = (void *)old_data.dptr;
>                         acc->memory = old_data.dsize;
>                         acc->domid = hdr->perms[0].id;
> @@ -655,6 +657,7 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key, 
> TDB_DATA *data,
>                 errno = EIO;
>                 return errno;
>         }
> +       trace_tdb("store %s size %zu\n", key->dptr, data->dsize + key->dsize);
>   
>   	if (acc) {
>                 /* Don't use new_domid, as it might be a transaction node. */
> @@ -682,6 +685,7 @@ int do_tdb_delete(struct connection *conn, TDB_DATA *key,
>                 errno = EIO;
>                 return errno;
>         }
> +       trace_tdb("delete %s\n", key->dptr);
>   
>   	if (acc->memory) {
>                 domid = get_acc_domid(conn, key, acc->domid);
> @@ -731,6 +735,8 @@ struct node *read_node(struct connection *conn, const void 
> *ctx,
>                 goto error;
>         }
>   
> +	trace_tdb("read %s size %zu\n", key.dptr, data.dsize + key.dsize);
> +
>         node->parent = NULL;
>         talloc_steal(node, data.dptr);
>   
> @@ -2746,7 +2752,7 @@ static void set_quota(const char *arg, bool soft)
>   
>   /* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
>   const char *const trace_switches[] = {
> -       "obj", "io", "wrl", "acc",
> +       "obj", "io", "wrl", "acc", "tdb",
>         NULL
>   };
>   
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index 3e0734a6c6..419a144396 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -303,8 +303,14 @@ extern unsigned int trace_flags;
>   #define TRACE_IO      0x00000002
>   #define TRACE_WRL     0x00000004
>   #define TRACE_ACC     0x00000008
> +#define TRACE_TDB      0x00000010
>   extern const char *const trace_switches[];
>   int set_trace_switch(const char *arg);
Add a newline here.

> +#define trace_tdb(...)                         \
> +do {                                           \
> +       if (trace_flags & TRACE_TDB)                \
> +               trace("tdb: " __VA_ARGS__);   \
> +} while (0)
>   
>   extern TDB_CONTEXT *tdb_ctx;
>   extern int dom0_domid;
> diff --git a/tools/xenstore/xenstored_transaction.c 
> b/tools/xenstore/xenstored_transaction.c
> index 1aa9d3cb3d..19a1175d1b 100644
> --- a/tools/xenstore/xenstored_transaction.c
> +++ b/tools/xenstore/xenstored_transaction.c
> @@ -366,8 +366,11 @@ static int finalize_transaction(struct connection *conn,
>                                 if (tdb_error(tdb_ctx) != TDB_ERR_NOEXIST)
>                                         return EIO;
>                                 gen = NO_GENERATION;
> -                       } else
> +                       } else {
> +                               trace_tdb("read %s size %zu\n", key.dptr,
> +                                         key.dsize + data.dsize);
>                                 gen = hdr->generation;
> +                       }
>                         talloc_free(data.dptr);
>                         if (i->generation != gen)
>                                 return EAGAIN;
> @@ -391,6 +394,8 @@ static int finalize_transaction(struct connection *conn,
>                         set_tdb_key(i->trans_name, &ta_key);
>                         data = tdb_fetch(tdb_ctx, ta_key);
>                         if (data.dptr) {
> +                               trace_tdb("read %s size %zu\n", ta_key.dptr,
> +                                         ta_key.dsize + data.dsize);
>                                 hdr = (void *)data.dptr;
>                                 hdr->generation = ++generation;
>                                 *is_corrupt |= do_tdb_write(conn, &key, &data,
Cheers,

--
Julien Grall