Re: [PATCH 02/10] tools/xenstored: Introduce lu_get_connection() and use it

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 16 Jun 2021, at 15:43, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> At the moment, dump_state_buffered_data() is taking two connections
> in parameters (one is the connection to dump, the other is the
> connection used to request LU). The naming doesn't help to
> distinguish (c vs conn) them and this already lead to several mistake
> while modifying the function.
> 
> To remove the confusion, introduce an help lu_get_connection() that
> will return the connection used to request LU and use it
> in place of the existing parameter.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>

> ---
> tools/xenstore/xenstored_control.c | 13 ++++++++++++-
> tools/xenstore/xenstored_control.h |  2 ++
> tools/xenstore/xenstored_core.c    |  7 +++----
> tools/xenstore/xenstored_core.h    |  1 -
> tools/xenstore/xenstored_domain.c  |  6 +++---
> tools/xenstore/xenstored_domain.h  |  2 +-
> 6 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_control.c 
> b/tools/xenstore/xenstored_control.c
> index 0d57f9f9400d..d08a2b961432 100644
> --- a/tools/xenstore/xenstored_control.c
> +++ b/tools/xenstore/xenstored_control.c
> @@ -104,6 +104,17 @@ static const char *lu_begin(struct connection *conn)
> 
>       return NULL;
> }
> +
> +struct connection *lu_get_connection(void)
> +{
> +     return lu_status ? lu_status->conn : NULL;
> +}
> +
> +#else
> +struct connection *lu_get_connection(void)
> +{
> +     return NULL;
> +}
> #endif
> 
> struct cmd_s {
> @@ -516,7 +527,7 @@ static const char *lu_dump_state(const void *ctx, struct 
> connection *conn)
>       ret = dump_state_global(fp);
>       if (ret)
>               goto out;
> -     ret = dump_state_connections(fp, conn);
> +     ret = dump_state_connections(fp);
>       if (ret)
>               goto out;
>       ret = dump_state_special_nodes(fp);
> diff --git a/tools/xenstore/xenstored_control.h 
> b/tools/xenstore/xenstored_control.h
> index aac61f05908f..6842b8d88760 100644
> --- a/tools/xenstore/xenstored_control.h
> +++ b/tools/xenstore/xenstored_control.h
> @@ -18,3 +18,5 @@
> 
> int do_control(struct connection *conn, struct buffered_data *in);
> void lu_read_state(void);
> +
> +struct connection *lu_get_connection(void);
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 883a1a582a60..607187361d84 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -2369,14 +2369,13 @@ const char *dump_state_global(FILE *fp)
> 
> /* Called twice: first with fp == NULL to get length, then for writing data. 
> */
> const char *dump_state_buffered_data(FILE *fp, const struct connection *c,
> -                                  const struct connection *conn,
>                                    struct xs_state_connection *sc)
> {
>       unsigned int len = 0, used;
>       struct buffered_data *out, *in = c->in;
>       bool partial = true;
> 
> -     if (in && c != conn) {
> +     if (in && c != lu_get_connection()) {
>               len = in->inhdr ? in->used : sizeof(in->hdr);
>               if (fp && fwrite(&in->hdr, len, 1, fp) != 1)
>                       return "Dump read data error";
> @@ -2416,8 +2415,8 @@ const char *dump_state_buffered_data(FILE *fp, const 
> struct connection *c,
>       }
> 
>       /* Add "OK" for live-update command. */
> -     if (c == conn) {
> -             struct xsd_sockmsg msg = conn->in->hdr.msg;
> +     if (c == lu_get_connection()) {
> +             struct xsd_sockmsg msg = c->in->hdr.msg;
> 
>               msg.len = sizeof("OK");
>               if (fp && fwrite(&msg, sizeof(msg), 1, fp) != 1)
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index bb36111ecc56..89ce155e755b 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -269,7 +269,6 @@ void set_tdb_key(const char *name, TDB_DATA *key);
> 
> const char *dump_state_global(FILE *fp);
> const char *dump_state_buffered_data(FILE *fp, const struct connection *c,
> -                                  const struct connection *conn,
>                                    struct xs_state_connection *sc);
> const char *dump_state_nodes(FILE *fp, const void *ctx);
> const char *dump_state_node_perms(FILE *fp, const struct xs_permissions 
> *perms,
> diff --git a/tools/xenstore/xenstored_domain.c 
> b/tools/xenstore/xenstored_domain.c
> index 322b0dbca449..6d8d29cbe41c 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -1183,7 +1183,7 @@ void wrl_apply_debit_trans_commit(struct connection 
> *conn)
>       wrl_apply_debit_actual(conn->domain);
> }
> 
> -const char *dump_state_connections(FILE *fp, struct connection *conn)
> +const char *dump_state_connections(FILE *fp)
> {
>       const char *ret = NULL;
>       unsigned int conn_id = 1;
> @@ -1209,7 +1209,7 @@ const char *dump_state_connections(FILE *fp, struct 
> connection *conn)
>                       sc.spec.socket_fd = c->fd;
>               }
> 
> -             ret = dump_state_buffered_data(NULL, c, conn, &sc);
> +             ret = dump_state_buffered_data(NULL, c, &sc);
>               if (ret)
>                       return ret;
>               head.length += sc.data_in_len + sc.data_out_len;
> @@ -1219,7 +1219,7 @@ const char *dump_state_connections(FILE *fp, struct 
> connection *conn)
>               if (fwrite(&sc, offsetof(struct xs_state_connection, data),
>                          1, fp) != 1)
>                       return "Dump connection state error";
> -             ret = dump_state_buffered_data(fp, c, conn, NULL);
> +             ret = dump_state_buffered_data(fp, c, NULL);
>               if (ret)
>                       return ret;
>               ret = dump_state_align(fp);
> diff --git a/tools/xenstore/xenstored_domain.h 
> b/tools/xenstore/xenstored_domain.h
> index cc5147d7e747..62ee471ea6aa 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -101,7 +101,7 @@ void wrl_log_periodic(struct wrl_timestampt now);
> void wrl_apply_debit_direct(struct connection *conn);
> void wrl_apply_debit_trans_commit(struct connection *conn);
> 
> -const char *dump_state_connections(FILE *fp, struct connection *conn);
> +const char *dump_state_connections(FILE *fp);
> const char *dump_state_special_nodes(FILE *fp);
> 
> void read_state_connection(const void *ctx, const void *state);
> -- 
> 2.17.1
> 
>