[lttng-dev] [RFC PATCH lttng-tools 15/18] Integrate serialized communication in lttng-ctl and sessiond

[Mathieu Desnoyers]

----- On Apr 18, 2019, at 12:18 PM, Yannick Lamarre ylamarre at efficios.com wrote:

> lttng-ctl and lttng-sessiond use serialized communication for
> messages using lttng_event.
> 
> Signed-off-by: Yannick Lamarre <ylamarre at efficios.com>
> ---
> src/bin/lttng-sessiond/client.c          | 23 ++++++++++++++++++++---
> src/common/sessiond-comm/sessiond-comm.h |  4 ++--
> src/lib/lttng-ctl/lttng-ctl.c            | 14 ++++++++++----
> 3 files changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c
> index 24e688c5..aed415ee 100644
> --- a/src/bin/lttng-sessiond/client.c
> +++ b/src/bin/lttng-sessiond/client.c
> @@ -1150,6 +1150,7 @@ error_add_context:
> 	case LTTNG_DISABLE_EVENT:
> 	{
> 
> +		struct lttng_event event;

I think we should remove the extra newline before the struct definition above
while we are there.

> 		/*
> 		 * FIXME: handle filter; for now we just receive the filter's
> 		 * bytecode along with the filter expression which are sent by
> @@ -1176,10 +1177,17 @@ error_add_context:
> 				count -= (size_t) ret;
> 			}
> 		}
> -		/* FIXME: passing packed structure to non-packed pointer */
> +		lttng_event_no_attr_deserialize(&event, &cmd_ctx->lsm->u.disable.event);
> +		if (cmd_ctx->lsm->domain.type == LTTNG_DOMAIN_KERNEL) {

Those if() would be neater in a switch() case.

> +			if (event.type == LTTNG_EVENT_PROBE || event.type == LTTNG_EVENT_FUNCTION ||
> event.type == LTTNG_EVENT_USERSPACE_PROBE) {
> +				lttng_event_probe_attr_deserialize(&event, &cmd_ctx->lsm->u.disable.event);
> +			} else if (event.type == LTTNG_EVENT_FUNCTION_ENTRY) {
> +				lttng_event_function_attr_deserialize(&event,
> &cmd_ctx->lsm->u.disable.event);
> +			}

else what ? Error handling ?

if () else if () without a following else typically hides missing error handling.

[...]

> -		ev = lttng_event_copy(&cmd_ctx->lsm->u.enable.event);
> +		lttng_event_no_attr_deserialize(&event, &cmd_ctx->lsm->u.disable.event);
> +		if (cmd_ctx->lsm->domain.type == LTTNG_DOMAIN_KERNEL) {
> +			if (event.type == LTTNG_EVENT_PROBE || event.type == LTTNG_EVENT_FUNCTION ||
> event.type == LTTNG_EVENT_USERSPACE_PROBE) {
> +				lttng_event_probe_attr_deserialize(&event, &cmd_ctx->lsm->u.disable.event);
> +			} else if (event.type == LTTNG_EVENT_FUNCTION_ENTRY) {
> +				lttng_event_function_attr_deserialize(&event,
> &cmd_ctx->lsm->u.disable.event);
> +			}

same.

> +		}
> +		ev = lttng_event_copy(&event);
> 		if (!ev) {
> 			DBG("Failed to copy event: %s",
> 					cmd_ctx->lsm->u.enable.event.name);
> diff --git a/src/common/sessiond-comm/sessiond-comm.h
> b/src/common/sessiond-comm/sessiond-comm.h
> index 78b18185..4c2240a0 100644
> --- a/src/common/sessiond-comm/sessiond-comm.h
> +++ b/src/common/sessiond-comm/sessiond-comm.h
> @@ -370,7 +370,7 @@ struct lttcomm_session_msg {
> 		/* Event data */
> 		struct {
> 			char channel_name[LTTNG_SYMBOL_NAME_LEN];
> -			struct lttng_event event LTTNG_PACKED;
> +			struct lttng_event_serialized event;
> 			/* Length of following filter expression. */
> 			uint32_t expression_len;
> 			/* Length of following bytecode for filter. */
> @@ -389,7 +389,7 @@ struct lttcomm_session_msg {
> 		} LTTNG_PACKED enable;
> 		struct {
> 			char channel_name[LTTNG_SYMBOL_NAME_LEN];
> -			struct lttng_event event LTTNG_PACKED;
> +			struct lttng_event_serialized event;
> 			/* Length of following filter expression. */
> 			uint32_t expression_len;
> 			/* Length of following bytecode for filter. */
> diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
> index 686a98ef..a9adb5c2 100644
> --- a/src/lib/lttng-ctl/lttng-ctl.c
> +++ b/src/lib/lttng-ctl/lttng-ctl.c
> @@ -1110,8 +1110,15 @@ int lttng_enable_event_with_exclusions(struct
> lttng_handle *handle,
> 	}
> 
> 	lttng_ctl_copy_lttng_domain(&lsm.domain, &handle->domain);
> -	/* FIXME: copying non-packed struct to packed struct. */
> -	memcpy(&lsm.u.enable.event, ev, sizeof(lsm.u.enable.event));
> +
> +	lttng_event_no_attr_serialize(&lsm.u.enable.event, ev);
> +	if (handle->domain.type == LTTNG_DOMAIN_KERNEL) {
> +		if (ev->type == LTTNG_EVENT_PROBE || ev->type == LTTNG_EVENT_FUNCTION ||
> ev->type == LTTNG_EVENT_USERSPACE_PROBE) {
> +			lttng_event_probe_attr_serialize(&lsm.u.enable.event, ev);
> +		} else if (ev->type == LTTNG_EVENT_FUNCTION_ENTRY) {
> +			lttng_event_function_attr_serialize(&lsm.u.enable.event, ev);
> +		}
> +	}

same.

Thanks,

Mathieu

> 
> 	lttng_ctl_copy_string(lsm.session.name, handle->session_name,
> 			sizeof(lsm.session.name));
> @@ -1310,8 +1317,7 @@ int lttng_disable_event_ext(struct lttng_handle *handle,
> 	lsm.cmd_type = LTTNG_DISABLE_EVENT;
> 
> 	lttng_ctl_copy_lttng_domain(&lsm.domain, &handle->domain);
> -	/* FIXME: copying non-packed struct to packed struct. */
> -	memcpy(&lsm.u.disable.event, ev, sizeof(lsm.u.disable.event));
> +	lttng_event_no_attr_serialize(&lsm.u.disable.event, ev);
> 
> 	lttng_ctl_copy_string(lsm.session.name, handle->session_name,
> 			sizeof(lsm.session.name));
> --
> 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