[lttng-dev] [RFC PATCH lttng-tools 18/18] Integrate serialized communication in lttng-ctl and sessiond
Mathieu Desnoyers
mathieu.desnoyers at efficios.com
Thu Apr 18 15:58:04 EDT 2019
----- On Apr 18, 2019, at 12:18 PM, Yannick Lamarre ylamarre at efficios.com wrote:
Comment about your patch titles: this one should describe that it
targets the snapshot output commands.
Currently, title-wise, it is the same as patch 15/18, which really does not
convey enough info.
Please review all patches from this patchset to ensure they each have a unique
title that clearly identifies what they target.
> lttng-ctl and lttng-sessiond use serialized communication for
> messages using lttng_snapshot_out.
out -> output
>
> Signed-off-by: Yannick Lamarre <ylamarre at efficios.com>
> ---
> src/bin/lttng-sessiond/client.c | 27 ++++++++++++++++++++++-----
> src/common/sessiond-comm/sessiond-comm.h | 4 ++--
> src/lib/lttng-ctl/snapshot.c | 24 ++++++++++++++++--------
> 3 files changed, 40 insertions(+), 15 deletions(-)
>
> diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c
> index aed415ee..43f17e56 100644
> --- a/src/bin/lttng-sessiond/client.c
> +++ b/src/bin/lttng-sessiond/client.c
> @@ -1760,9 +1760,12 @@ error_add_context:
> case LTTNG_SNAPSHOT_ADD_OUTPUT:
> {
> struct lttcomm_lttng_output_id reply;
> + struct lttng_snapshot_output snapshot_output;
> +
> + lttng_snapshot_output_deserialize(&snapshot_output,
> &cmd_ctx->lsm->u.snapshot_output.output);
>
> ret = cmd_snapshot_add_output(cmd_ctx->session,
> - &cmd_ctx->lsm->u.snapshot_output.output, &reply.id);
> + &snapshot_output, &reply.id);
> if (ret != LTTNG_OK) {
> goto error;
> }
> @@ -1779,14 +1782,19 @@ error_add_context:
> }
> case LTTNG_SNAPSHOT_DEL_OUTPUT:
> {
> + struct lttng_snapshot_output snapshot_output;
> +
> + lttng_snapshot_output_deserialize(&snapshot_output,
> &cmd_ctx->lsm->u.snapshot_output.output);
> +
> ret = cmd_snapshot_del_output(cmd_ctx->session,
> - &cmd_ctx->lsm->u.snapshot_output.output);
> + &snapshot_output);
> break;
> }
> case LTTNG_SNAPSHOT_LIST_OUTPUT:
> {
> ssize_t nb_output;
> struct lttng_snapshot_output *outputs = NULL;
> + struct lttng_snapshot_output_serialized *serialized_outputs;
>
> nb_output = cmd_snapshot_list_outputs(cmd_ctx->session, &outputs);
> if (nb_output < 0) {
> @@ -1795,9 +1803,14 @@ error_add_context:
> }
>
> assert((nb_output > 0 && outputs) || nb_output == 0);
> - ret = setup_lttng_msg_no_cmd_header(cmd_ctx, outputs,
> - nb_output * sizeof(struct lttng_snapshot_output));
> + serialized_outputs = malloc(sizeof(struct lttng_snapshot_output_serialized) *
> nb_output);
> + for (int i = 0; i < nb_output; i++) {
We don't use for (int i = ... in our coding style. Please define int i in the scope containing
the for ().
> + lttng_snapshot_output_serialize(&serialized_outputs[i], &outputs[i]);
> + }
> free(outputs);
> + ret = setup_lttng_msg_no_cmd_header(cmd_ctx, serialized_outputs,
> + nb_output * sizeof(struct lttng_snapshot_output_serialized));
> + free(serialized_outputs);
>
> if (ret < 0) {
> goto setup_error;
> @@ -1808,8 +1821,12 @@ error_add_context:
> }
> case LTTNG_SNAPSHOT_RECORD:
> {
> + struct lttng_snapshot_output snapshot_output;
> +
> + lttng_snapshot_output_deserialize(&snapshot_output,
> &cmd_ctx->lsm->u.snapshot_output.output);
> +
> ret = cmd_snapshot_record(cmd_ctx->session,
> - &cmd_ctx->lsm->u.snapshot_record.output,
> + &snapshot_output,
> cmd_ctx->lsm->u.snapshot_record.wait);
> break;
> }
> diff --git a/src/common/sessiond-comm/sessiond-comm.h
> b/src/common/sessiond-comm/sessiond-comm.h
> index 4c2240a0..a21510de 100644
> --- a/src/common/sessiond-comm/sessiond-comm.h
> +++ b/src/common/sessiond-comm/sessiond-comm.h
> @@ -428,11 +428,11 @@ struct lttcomm_session_msg {
> uint32_t size;
> } LTTNG_PACKED uri;
> struct {
> - struct lttng_snapshot_output output LTTNG_PACKED;
> + struct lttng_snapshot_output_serialized output;
> } LTTNG_PACKED snapshot_output;
> struct {
> uint32_t wait;
> - struct lttng_snapshot_output output LTTNG_PACKED;
> + struct lttng_snapshot_output_serialized output;
> } LTTNG_PACKED snapshot_record;
> struct {
> uint32_t nb_uri;
> diff --git a/src/lib/lttng-ctl/snapshot.c b/src/lib/lttng-ctl/snapshot.c
> index b30c4706..9c015bc2 100644
> --- a/src/lib/lttng-ctl/snapshot.c
> +++ b/src/lib/lttng-ctl/snapshot.c
> @@ -47,8 +47,7 @@ int lttng_snapshot_add_output(const char *session_name,
>
> lttng_ctl_copy_string(lsm.session.name, session_name,
> sizeof(lsm.session.name));
> - memcpy(&lsm.u.snapshot_output.output, output,
> - sizeof(lsm.u.snapshot_output.output));
> + lttng_snapshot_output_serialize(&lsm.u.snapshot_output.output, output);
>
> ret = lttng_ctl_ask_sessiond(&lsm, (void **) &reply);
> if (ret < 0) {
> @@ -80,8 +79,7 @@ int lttng_snapshot_del_output(const char *session_name,
>
> lttng_ctl_copy_string(lsm.session.name, session_name,
> sizeof(lsm.session.name));
> - memcpy(&lsm.u.snapshot_output.output, output,
> - sizeof(lsm.u.snapshot_output.output));
> + lttng_snapshot_output_serialize(&lsm.u.snapshot_output.output, output);
>
> return lttng_ctl_ask_sessiond(&lsm, NULL);
> }
> @@ -99,6 +97,7 @@ int lttng_snapshot_list_output(const char *session_name,
> int ret;
> struct lttcomm_session_msg lsm;
> struct lttng_snapshot_output_list *new_list = NULL;
> + struct lttng_snapshot_output_serialized *serialized_array;
>
> if (!session_name || !list) {
> ret = -LTTNG_ERR_INVALID;
> @@ -117,12 +116,22 @@ int lttng_snapshot_list_output(const char *session_name,
> goto error;
> }
>
> - ret = lttng_ctl_ask_sessiond(&lsm, (void **) &new_list->array);
> + ret = lttng_ctl_ask_sessiond(&lsm, (void **) &serialized_array);
> if (ret < 0) {
> goto free_error;
> }
>
> - new_list->count = ret / sizeof(struct lttng_snapshot_output);
> + new_list->count = ret / sizeof(struct lttng_snapshot_output_serialized);
> + new_list->array = zmalloc(sizeof(struct lttng_snapshot_output) *
> new_list->count);
> + if (!new_list) {
> + ret = -LTTNG_ERR_NOMEM;
> + goto free_error;
> + }
> + for (int i = 0; i < new_list->count; i++) {
likewise for "int i".
Also, since we are here: it's a bonus point if we assign
new_list->count to a local variable and use it as end of loop
comparison rather than dereferencing the new_list-> pointer
for each loop.
Thanks,
Mathieu
> + lttng_snapshot_output_deserialize(&new_list->array[i], &serialized_array[i]);
> + }
> + free(serialized_array);
> +
> *list = new_list;
> return 0;
>
> @@ -207,8 +216,7 @@ int lttng_snapshot_record(const char *session_name,
> * record.
> */
> if (output) {
> - memcpy(&lsm.u.snapshot_record.output, output,
> - sizeof(lsm.u.snapshot_record.output));
> + lttng_snapshot_output_serialize(&lsm.u.snapshot_record.output, output);
> }
>
> /* The wait param is ignored. */
> --
> 2.11.0
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list