[RFC PATCH lttng-tools 01/18] Fix: Use platform-independant types in sessiond to consumerd communication

[RFC PATCH lttng-tools 01/18] Fix: Use platform-independant types in sessiond to consumerd communication

[Yannick Lamarre]

From: Jérémie Galarneau <jeremie.galarneau@efficios.com>

The session daemon to consumer daemon is passin around the
lttcomm_consumer_msg structure which contains a relayd_sock
structure.

This structure, in turn, contains a lttcomm_relayd_sock structure which
contains an lttcomm_sock structure.

If you're still following, this lttcomm_sock structure has an
"ops" member which is a pointer. Obviously, this pointer does not have
the same size between a 64-bit session daemon and a 32-bit consumer
which causes communication issues when adding a relayd socket (the
LTTng version is erronously interpreted as 0.2).

This fix introduces "serialized" versions of the aforementioned
structures which only use fixed-size members. Additionaly, these
"serialized" structures' members are stored as big-endian and the
rest of the communication should transition to this convention.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/bin/lttng-sessiond/consumer.c            |   5 +-
 src/common/kernel-consumer/kernel-consumer.c |  11 +-
 src/common/sessiond-comm/sessiond-comm.c     | 186 +++++++++++++++++++++++++++
 src/common/sessiond-comm/sessiond-comm.h     | 111 +++++++++++++++-
 src/common/ust-consumer/ust-consumer.c       |  11 +-
 tests/regression/tools/live/Makefile.am      |   2 +-
 tests/unit/Makefile.am                       |   6 +-
 7 files changed, 323 insertions(+), 9 deletions(-)

diff --git a/src/bin/lttng-sessiond/consumer.c b/src/bin/lttng-sessiond/consumer.c
index 80e31377..d22b0cbc 100644
--- a/src/bin/lttng-sessiond/consumer.c
+++ b/src/bin/lttng-sessiond/consumer.c
@@ -1048,7 +1048,10 @@ int consumer_send_relayd_socket(struct consumer_socket *consumer_sock,
 	msg.u.relayd_sock.net_index = consumer->net_seq_index;
 	msg.u.relayd_sock.type = type;
 	msg.u.relayd_sock.session_id = session_id;
-	memcpy(&msg.u.relayd_sock.sock, rsock, sizeof(msg.u.relayd_sock.sock));
+	ret = lttcomm_relayd_sock_serialize(&msg.u.relayd_sock.sock, rsock);
+	if (ret) {
+		goto error;
+	}
 
 	DBG3("Sending relayd sock info to consumer on %d", *consumer_sock->fd_ptr);
 	ret = consumer_send_msg(consumer_sock, &msg);
diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
index 627cd2a8..16e3b588 100644
--- a/src/common/kernel-consumer/kernel-consumer.c
+++ b/src/common/kernel-consumer/kernel-consumer.c
@@ -454,10 +454,19 @@ int lttng_kconsumer_recv_cmd(struct lttng_consumer_local_data *ctx,
 	switch (msg.cmd_type) {
 	case LTTNG_CONSUMER_ADD_RELAYD_SOCKET:
 	{
+		struct lttcomm_relayd_sock relayd_sock;
+
+		ret = lttcomm_relayd_sock_deserialize(&relayd_sock,
+				&msg.u.relayd_sock.sock);
+		if (ret) {
+			/* Received an invalid relayd_sock. */
+			goto error_fatal;
+		}
+
 		/* Session daemon status message are handled in the following call. */
 		consumer_add_relayd_socket(msg.u.relayd_sock.net_index,
 				msg.u.relayd_sock.type, ctx, sock, consumer_sockpoll,
-				&msg.u.relayd_sock.sock, msg.u.relayd_sock.session_id,
+				&relayd_sock, msg.u.relayd_sock.session_id,
 				msg.u.relayd_sock.relayd_session_id);
 		goto end_nosignal;
 	}
diff --git a/src/common/sessiond-comm/sessiond-comm.c b/src/common/sessiond-comm/sessiond-comm.c
index 0067be16..8aee3cd1 100644
--- a/src/common/sessiond-comm/sessiond-comm.c
+++ b/src/common/sessiond-comm/sessiond-comm.c
@@ -27,6 +27,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <inttypes.h>
+#include <endian.h>
 
 #include <common/common.h>
 
@@ -75,6 +76,191 @@ static const char *lttcomm_readable_code[] = {
 
 static unsigned long network_timeout;
 
+LTTNG_HIDDEN
+int sockaddr_in_serialize(struct sockaddr_in_serialized *dst,
+		const struct sockaddr_in *src)
+{
+	assert(src && dst);
+	dst->sin_family = (uint32_t) src->sin_family;
+	dst->sin_port = (uint16_t) src->sin_port;
+	dst->sin_addr.s_addr = src->sin_addr.s_addr;
+	return 0;
+}
+
+LTTNG_HIDDEN
+int sockaddr_in_deserialize(struct sockaddr_in *dst,
+		const struct sockaddr_in_serialized *src)
+{
+	assert(src && dst);
+	dst->sin_family = (sa_family_t) src->sin_family;
+	dst->sin_port = (in_port_t) src->sin_port;
+	dst->sin_addr.s_addr = src->sin_addr.s_addr;
+	return 0;
+}
+
+LTTNG_HIDDEN
+int sockaddr_in6_serialize(struct sockaddr_in6_serialized *dst,
+		const struct sockaddr_in6 *src)
+{
+	assert(src && dst);
+
+	dst->sin6_family = (uint32_t) src->sin6_family;
+	dst->sin6_port = (uint16_t) src->sin6_port;
+	dst->sin6_flowinfo = src->sin6_flowinfo;
+	memcpy(&dst->sin6_addr._s6_addr, src->sin6_addr.s6_addr,
+			sizeof(dst->sin6_addr._s6_addr));
+	dst->sin6_scope_id = src->sin6_scope_id;
+	return 0;
+}
+
+LTTNG_HIDDEN
+int sockaddr_in6_deserialize(struct sockaddr_in6 *dst,
+		const struct sockaddr_in6_serialized *src)
+{
+	assert(src && dst);
+
+	dst->sin6_family = (sa_family_t) src->sin6_family;
+	dst->sin6_port = (in_port_t) src->sin6_port;
+	dst->sin6_flowinfo = src->sin6_flowinfo;
+	memcpy(&dst->sin6_addr.s6_addr, src->sin6_addr._s6_addr,
+	       sizeof(dst->sin6_addr.s6_addr));
+	dst->sin6_scope_id = src->sin6_scope_id;
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttcomm_sockaddr_serialize(struct lttcomm_sockaddr_serialized *dst,
+		const struct lttcomm_sockaddr *src)
+{
+	int ret = 0;
+
+	assert(src && dst);
+
+	dst->type = (uint32_t) src->type;
+
+	switch (src->type) {
+	case LTTCOMM_INET:
+	{
+		sockaddr_in_serialize(&dst->addr.sin,
+				&src->addr.sin);
+		break;
+	}
+	case LTTCOMM_INET6:
+	{
+		sockaddr_in6_serialize(&dst->addr.sin6,
+				&src->addr.sin6);
+		break;
+	}
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+LTTNG_HIDDEN
+int lttcomm_sockaddr_deserialize(struct lttcomm_sockaddr *dst,
+		const struct lttcomm_sockaddr_serialized *src)
+{
+	int ret = 0;
+
+	assert(src && dst);
+
+	dst->type = (enum lttcomm_sock_domain) src->type;
+
+	switch (dst->type) {
+	case LTTCOMM_INET:
+	{
+		sockaddr_in_deserialize(&dst->addr.sin,
+				&src->addr.sin);
+		break;
+	}
+	case LTTCOMM_INET6:
+	{
+		sockaddr_in6_deserialize(&dst->addr.sin6,
+				&src->addr.sin6);
+		break;
+	}
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+LTTNG_HIDDEN
+int lttcomm_sock_serialize(struct lttcomm_sock_serialized *dst,
+		const struct lttcomm_sock *src)
+{
+	int ret;
+
+	assert(src && dst);
+
+	dst->fd = src->fd;
+	if (src->proto != LTTCOMM_SOCK_UDP &&
+		src->proto != LTTCOMM_SOCK_TCP) {
+		/* Code flow error. */
+		assert(0);
+	}
+	dst->proto = (uint32_t) src->proto;
+	ret = lttcomm_sockaddr_serialize(&dst->sockaddr, &src->sockaddr);
+
+	return ret;
+}
+
+LTTNG_HIDDEN
+int lttcomm_sock_deserialize(struct lttcomm_sock *dst,
+		const struct lttcomm_sock_serialized *src)
+{
+	int ret;
+
+	assert(src && dst);
+
+	dst->fd = src->fd;
+	dst->proto = (enum lttcomm_sock_proto) src->proto;
+	if (dst->proto != LTTCOMM_SOCK_UDP &&
+		dst->proto != LTTCOMM_SOCK_TCP) {
+		ret = -EINVAL;
+		goto end;
+	}
+	dst->ops = NULL;
+	ret = lttcomm_sockaddr_deserialize(&dst->sockaddr, &src->sockaddr);
+
+end:
+	return ret;
+}
+
+LTTNG_HIDDEN
+int lttcomm_relayd_sock_serialize(struct lttcomm_relayd_sock_serialized *dst,
+		const struct lttcomm_relayd_sock *src)
+{
+	int ret;
+
+	assert(src && dst);
+	dst->major = src->major;
+	dst->minor = src->minor;
+	ret = lttcomm_sock_serialize(&dst->sock, &src->sock);
+
+	return ret;
+}
+
+LTTNG_HIDDEN
+int lttcomm_relayd_sock_deserialize(
+		struct lttcomm_relayd_sock *dst,
+		const struct lttcomm_relayd_sock_serialized *src)
+{
+	int ret;
+
+	assert(src && dst);
+	dst->major = src->major;
+	dst->minor = src->minor;
+	ret = lttcomm_sock_deserialize(&dst->sock, &src->sock);
+
+	return ret;
+}
+
 /*
  * Return ptr to string representing a human readable error code from the
  * lttcomm_return_code enum.
diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index c0c6a8bb..556c277f 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -2,6 +2,7 @@
  * Copyright (C) 2011 - David Goulet <david.goulet@polymtl.ca>
  *                      Julien Desfossez <julien.desfossez@polymtl.ca>
  *                      Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *                      Jérémie Galarneau <jeremie.galarneau@efficios.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License, version 2 only,
@@ -209,31 +210,137 @@ struct lttcomm_metadata_request_msg {
 	uint64_t key; /* Metadata channel key. */
 } LTTNG_PACKED;
 
+/* For internal use only */
 struct lttcomm_sockaddr {
 	enum lttcomm_sock_domain type;
 	union {
 		struct sockaddr_in sin;
 		struct sockaddr_in6 sin6;
 	} addr;
+};
+
+/*
+ * Serialized version of the sockaddr_in system structure (may only be used
+ * for communication).
+ * Fields are fixed-size, big endian and packed.
+ */
+struct sockaddr_in_serialized {
+	uint32_t sin_family;
+	uint16_t sin_port;
+	struct in_addr_serialized {
+		uint32_t s_addr;
+	} LTTNG_PACKED sin_addr;
+} LTTNG_PACKED;
+
+extern
+int sockaddr_in_serialize(struct sockaddr_in_serialized *dst,
+		const struct sockaddr_in *src);
+extern
+int sockaddr_in_deserialize(struct sockaddr_in *dst,
+		const struct sockaddr_in_serialized *src);
+
+/*
+ * Serialized version of the sockaddr_in6 system structure (may only be used
+ * for communication).
+ * Fields are fixed-size, big endian and packed.
+ */
+struct sockaddr_in6_serialized {
+	uint32_t sin6_family;
+	uint16_t sin6_port;
+	uint32_t sin6_flowinfo;
+	struct in6_addr_serialized {
+		/*
+		 * Prefixing with "_" since s6_addr is a "DEFINE"
+		 * which clashes with this.
+		 */
+		uint8_t _s6_addr[16];
+	} LTTNG_PACKED sin6_addr;
+	uint32_t sin6_scope_id;
+} LTTNG_PACKED;
+
+extern
+int sockaddr_in6_serialize(struct sockaddr_in6_serialized *dst,
+		const struct sockaddr_in6 *src);
+extern
+int sockaddr_in6_deserialize(struct sockaddr_in6 *dst,
+		const struct sockaddr_in6_serialized *src);
+
+/*
+ * Serialized version of struct lttcomm_sockaddr (may be used for
+ * communication only).
+ * The struct and its members are packed and its fields are fixed-size, big
+ * endian.
+ */
+struct lttcomm_sockaddr_serialized {
+	uint32_t type; /* Maps to enum lttcomm_sock_domain */
+	union {
+		struct sockaddr_in_serialized sin;
+		struct sockaddr_in6_serialized sin6;
+	} addr;
 } LTTNG_PACKED;
 
+extern
+int sockaddr_in6_serialize(struct sockaddr_in6_serialized *dst,
+		const struct sockaddr_in6 *src);
+extern
+int sockaddr_in6_deserialize(struct sockaddr_in6 *dst,
+		const struct sockaddr_in6_serialized *src);
+
+/* For internal use only */
 struct lttcomm_sock {
 	int32_t fd;
 	enum lttcomm_sock_proto proto;
 	struct lttcomm_sockaddr sockaddr;
 	const struct lttcomm_proto_ops *ops;
+};
+
+/*
+ * Serialized version of struct lttcomm_sock (may be used for
+ * communication only).
+ * Fields are fixed-size, big endian. Structure is packed.
+ */
+struct lttcomm_sock_serialized {
+	int32_t fd;
+	uint32_t proto; /* Maps to enum lttcomm_sock_proto */
+	struct lttcomm_sockaddr_serialized sockaddr;
 } LTTNG_PACKED;
 
+extern
+int lttcomm_sock_serialize(struct lttcomm_sock_serialized *dst,
+		const struct lttcomm_sock *src);
+extern
+int lttcomm_sock_deserialize(struct lttcomm_sock *dst,
+		const struct lttcomm_sock_serialized *src);
+
 /*
  * Relayd sock. Adds the protocol version to use for the communications with
- * the relayd.
+ * the relayd. Internal use only.
  */
 struct lttcomm_relayd_sock {
 	struct lttcomm_sock sock;
 	uint32_t major;
 	uint32_t minor;
+};
+
+/*
+ * Serialized version of struct lttcomm_relayd_sock (may be used for
+ * communications only).
+ * Fields are fixed-size, big endian. Structure is packed.
+ */
+struct lttcomm_relayd_sock_serialized {
+	struct lttcomm_sock_serialized sock;
+	uint32_t major;
+	uint32_t minor;
 } LTTNG_PACKED;
 
+extern
+int lttcomm_relayd_sock_serialize(struct lttcomm_relayd_sock_serialized *dst,
+		const struct lttcomm_relayd_sock *src);
+extern
+int lttcomm_relayd_sock_deserialize(
+		struct lttcomm_relayd_sock *dst,
+		const struct lttcomm_relayd_sock_serialized *src);
+
 struct lttcomm_net_family {
 	int family;
 	int (*create) (struct lttcomm_sock *sock, int type, int proto);
@@ -486,7 +593,7 @@ struct lttcomm_consumer_msg {
 			uint64_t net_index;
 			enum lttng_stream_type type;
 			/* Open socket to the relayd */
-			struct lttcomm_relayd_sock sock;
+			struct lttcomm_relayd_sock_serialized sock;
 			/* Tracing session id associated to the relayd. */
 			uint64_t session_id;
 			/* Relayd session id, only used with control socket. */
diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
index 94b761cb..06f858c3 100644
--- a/src/common/ust-consumer/ust-consumer.c
+++ b/src/common/ust-consumer/ust-consumer.c
@@ -1354,10 +1354,19 @@ int lttng_ustconsumer_recv_cmd(struct lttng_consumer_local_data *ctx,
 	switch (msg.cmd_type) {
 	case LTTNG_CONSUMER_ADD_RELAYD_SOCKET:
 	{
+		struct lttcomm_relayd_sock relayd_sock;
+
+		ret = lttcomm_relayd_sock_deserialize(&relayd_sock,
+				&msg.u.relayd_sock.sock);
+		if (ret) {
+			/* Received an invalid relayd_sock. */
+			goto error_fatal;
+		}
+
 		/* Session daemon status message are handled in the following call. */
 		consumer_add_relayd_socket(msg.u.relayd_sock.net_index,
 				msg.u.relayd_sock.type, ctx, sock, consumer_sockpoll,
-				&msg.u.relayd_sock.sock, msg.u.relayd_sock.session_id,
+				&relayd_sock, msg.u.relayd_sock.session_id,
 				msg.u.relayd_sock.relayd_session_id);
 		goto end_nosignal;
 	}
diff --git a/tests/regression/tools/live/Makefile.am b/tests/regression/tools/live/Makefile.am
index 7a26b42c..857463e6 100644
--- a/tests/regression/tools/live/Makefile.am
+++ b/tests/regression/tools/live/Makefile.am
@@ -20,7 +20,7 @@ EXTRA_DIST += test_ust test_ust_tracefile_count test_lttng_ust
 endif
 
 live_test_SOURCES = live_test.c
-live_test_LDADD = $(LIBTAP) $(LIBCOMMON) $(LIBRELAYD) $(LIBSESSIOND_COMM) \
+live_test_LDADD = $(LIBTAP) $(LIBSESSIOND_COMM) $(LIBCOMMON) $(LIBRELAYD) \
 		$(LIBHASHTABLE) $(LIBHEALTH) $(DL_LIBS) -lrt
 live_test_LDADD += $(LIVE) \
 		$(top_builddir)/src/lib/lttng-ctl/liblttng-ctl.la
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index 5f485f00..26989fa0 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -88,7 +88,7 @@ SESSIOND_OBJS += $(top_builddir)/src/bin/lttng-sessiond/trace-ust.$(OBJEXT) \
 endif
 
 test_session_SOURCES = test_session.c
-test_session_LDADD = $(LIBTAP) $(LIBCOMMON) $(LIBRELAYD) $(LIBSESSIOND_COMM) \
+test_session_LDADD = $(LIBTAP) $(LIBSESSIOND_COMM) $(LIBCOMMON) $(LIBRELAYD) \
 		     $(LIBHASHTABLE) $(DL_LIBS) -lrt -lurcu-common -lurcu \
 		     $(KMOD_LIBS) \
 		     $(top_builddir)/src/lib/lttng-ctl/liblttng-ctl.la \
@@ -108,7 +108,7 @@ endif
 # UST data structures unit test
 if HAVE_LIBLTTNG_UST_CTL
 test_ust_data_SOURCES = test_ust_data.c
-test_ust_data_LDADD = $(LIBTAP) $(LIBCOMMON) $(LIBRELAYD) $(LIBSESSIOND_COMM) \
+test_ust_data_LDADD = $(LIBTAP) $(LIBSESSIOND_COMM) $(LIBCOMMON) $(LIBRELAYD) \
 		      $(LIBHASHTABLE) $(DL_LIBS) -lrt -lurcu-common -lurcu \
 		      -llttng-ust-ctl \
 		      $(KMOD_LIBS) \
@@ -131,7 +131,7 @@ KERN_DATA_TRACE=$(top_builddir)/src/bin/lttng-sessiond/trace-kernel.$(OBJEXT) \
 		$(LIBLTTNG_CTL)
 
 test_kernel_data_SOURCES = test_kernel_data.c
-test_kernel_data_LDADD = $(LIBTAP) $(LIBCOMMON) $(LIBRELAYD) $(LIBSESSIOND_COMM) \
+test_kernel_data_LDADD = $(LIBTAP) $(LIBSESSIOND_COMM) $(LIBCOMMON) $(LIBRELAYD) \
 			 $(LIBHASHTABLE) $(DL_LIBS) -lrt
 test_kernel_data_LDADD += $(KERN_DATA_TRACE)
 
-- 
2.11.0

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[RFC PATCH lttng-tools 02/18] Clean-up: Switch enum fields in lttcomm_consumer_msg

[Yannick Lamarre]

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/sessiond-comm/sessiond-comm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index 556c277f..43a57736 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -564,7 +564,7 @@ struct lttcomm_consumer_msg {
 			uint32_t nb_init_streams;
 			char name[LTTNG_SYMBOL_NAME_LEN];
 			/* Use splice or mmap to consume this fd */
-			enum lttng_event_output output;
+			uint32_t output; /* enum lttng_event_output */
 			int type; /* Per cpu or metadata. */
 			uint64_t tracefile_size; /* bytes */
 			uint32_t tracefile_count; /* number of tracefiles */
@@ -591,7 +591,7 @@ struct lttcomm_consumer_msg {
 		} LTTNG_PACKED stream;	/* Only used by Kernel. */
 		struct {
 			uint64_t net_index;
-			enum lttng_stream_type type;
+			uint32_t type; /* enum lttng_stream_type */
 			/* Open socket to the relayd */
 			struct lttcomm_relayd_sock_serialized sock;
 			/* Tracing session id associated to the relayd. */
-- 
2.11.0

[RFC PATCH lttng-tools 03/18] Clean-up: Switch enum fields in lttcomm_consumer_status_msg

[Yannick Lamarre]

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/sessiond-comm/sessiond-comm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index 43a57736..1324f0b3 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -756,7 +756,7 @@ struct lttcomm_consumer_channel_monitor_msg {
  * Status message returned to the sessiond after a received command.
  */
 struct lttcomm_consumer_status_msg {
-	enum lttcomm_return_code ret_code;
+	uint32_t ret_code; /* enum lttcomm_return_code */
 } LTTNG_PACKED;
 
 struct lttcomm_consumer_status_channel {
-- 
2.11.0

[RFC PATCH lttng-tools 04/18] Clean-up: Switch enum fields in lttcomm_consumer_status_channel

[Yannick Lamarre]

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/sessiond-comm/sessiond-comm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index 1324f0b3..92cab8e6 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -760,7 +760,7 @@ struct lttcomm_consumer_status_msg {
 } LTTNG_PACKED;
 
 struct lttcomm_consumer_status_channel {
-	enum lttcomm_return_code ret_code;
+	uint32_t ret_code; /* enum lttcomm_return_code */
 	uint64_t key;
 	unsigned int stream_count;
 } LTTNG_PACKED;
-- 
2.11.0

[RFC PATCH lttng-tools 05/18] Clean-up: Remove unused structure

[Yannick Lamarre]

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/sessiond-comm/sessiond-comm.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index 92cab8e6..eb5d8ecb 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -770,21 +770,6 @@ struct lttcomm_consumer_status_channel {
 #include <lttng/ust-abi.h>
 
 /*
- * Data structure for the commands sent from sessiond to UST.
- */
-struct lttcomm_ust_msg {
-	uint32_t handle;
-	uint32_t cmd;
-	union {
-		struct lttng_ust_channel channel;
-		struct lttng_ust_stream stream;
-		struct lttng_ust_event event;
-		struct lttng_ust_context context;
-		struct lttng_ust_tracer_version version;
-	} u;
-} LTTNG_PACKED;
-
-/*
  * Data structure for the response from UST to the session daemon.
  * cmd_type is sent back in the reply for validation.
  */
-- 
2.11.0

[RFC PATCH lttng-tools 06/18] Clean-up: Removed deprecated field

[Yannick Lamarre]

The command using this command was dropped in 2.9, but the field was
left in the union.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/sessiond-comm/sessiond-comm.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index eb5d8ecb..23e82c8a 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -421,7 +421,6 @@ struct lttcomm_session_msg {
 		struct {
 			char channel_name[LTTNG_SYMBOL_NAME_LEN];
 		} LTTNG_PACKED list;
-		struct lttng_calibrate calibrate;
 		/* Used by the set_consumer_url and used by create_session also call */
 		struct {
 			/* Number of lttng_uri following */
-- 
2.11.0

[RFC PATCH lttng-tools 07/18] Add serialized versions of lttng_channel structs

[Yannick Lamarre]

Serialized versions of lttng_channel and lttng_channel_extended are
packed structures to be used in communication protocols for consistent
sizes across platforms. The serialized versions are stripped of pointers
and padding.

Pointers are removed since their size can vary on platforms supporting
variable sized registers (x86-64).
Padding is also removed since it defeats the purpose of a packed struct.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/channel-internal.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/lttng/channel-internal.h b/include/lttng/channel-internal.h
index 030b4701..c956c19d 100644
--- a/include/lttng/channel-internal.h
+++ b/include/lttng/channel-internal.h
@@ -20,11 +20,41 @@
 
 #include <common/macros.h>
 
+struct lttng_channel_attr_serialized {
+	int overwrite;                      /* -1: session default, 1: overwrite, 0: discard */
+	uint64_t subbuf_size;               /* bytes, power of 2 */
+	uint64_t num_subbuf;                /* power of 2 */
+	unsigned int switch_timer_interval; /* usec */
+	unsigned int read_timer_interval;   /* usec */
+	uint32_t output; /* enum lttng_event_output */
+	/* LTTng 2.1 padding limit */
+	uint64_t tracefile_size;            /* bytes */
+	uint64_t tracefile_count;           /* number of tracefiles */
+	/* LTTng 2.3 padding limit */
+	unsigned int live_timer_interval;   /* usec */
+	/* LTTng 2.7 padding limit */
+
+} LTTNG_PACKED;
+
+struct lttng_channel_serialized {
+	char name[LTTNG_SYMBOL_NAME_LEN];
+	uint32_t enabled;
+	struct lttng_channel_attr_serialized attr;
+
+} LTTNG_PACKED;
+
 struct lttng_channel_extended {
 	uint64_t discarded_events;
 	uint64_t lost_packets;
 	uint64_t monitor_timer_interval;
 	int64_t blocking_timeout;
+};
+
+struct lttng_channel_extended_serialized {
+	uint64_t discarded_events;
+	uint64_t lost_packets;
+	uint64_t monitor_timer_interval;
+	int64_t blocking_timeout;
 } LTTNG_PACKED;
 
 #endif /* LTTNG_CHANNEL_INTERNAL_H */
-- 
2.11.0

[RFC PATCH lttng-tools 08/18] Add serdes functions for lttng_channel

[Yannick Lamarre]

Since those structs are only transfered across unix sockets, endianess
is kept in host order.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/channel-internal.h         |   5 ++
 src/common/sessiond-comm/sessiond-comm.c | 105 +++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/include/lttng/channel-internal.h b/include/lttng/channel-internal.h
index c956c19d..db963f13 100644
--- a/include/lttng/channel-internal.h
+++ b/include/lttng/channel-internal.h
@@ -57,4 +57,9 @@ struct lttng_channel_extended_serialized {
 	int64_t blocking_timeout;
 } LTTNG_PACKED;
 
+int lttng_channel_serialize(struct lttng_channel_serialized *dst, const struct lttng_channel *src);
+int lttng_channel_deserialize(struct lttng_channel *dst, const struct lttng_channel_serialized *src);
+int lttng_channel_extended_deserialize(struct lttng_channel_extended *dst, const struct lttng_channel_extended_serialized *src);
+int lttng_channel_extended_serialize(struct lttng_channel_extended_serialized *dst, const struct lttng_channel_extended *src);
+int init_serialized_extended_channel(struct lttng_domain *domain, struct lttng_channel_extended_serialized *extended);
 #endif /* LTTNG_CHANNEL_INTERNAL_H */
diff --git a/src/common/sessiond-comm/sessiond-comm.c b/src/common/sessiond-comm/sessiond-comm.c
index 8aee3cd1..6dc58739 100644
--- a/src/common/sessiond-comm/sessiond-comm.c
+++ b/src/common/sessiond-comm/sessiond-comm.c
@@ -76,6 +76,111 @@ static const char *lttcomm_readable_code[] = {
 
 static unsigned long network_timeout;
 
+int init_serialized_extended_channel(struct lttng_domain *domain, struct
+		lttng_channel_extended_serialized *extended)
+{
+	assert(domain && extended);
+	switch (domain->type) {
+	case LTTNG_DOMAIN_KERNEL:
+		extended->monitor_timer_interval =
+			DEFAULT_KERNEL_CHANNEL_MONITOR_TIMER;
+		extended->blocking_timeout =
+			DEFAULT_KERNEL_CHANNEL_BLOCKING_TIMEOUT;
+		break;
+	case LTTNG_DOMAIN_UST:
+		switch (domain->buf_type) {
+		case LTTNG_BUFFER_PER_UID:
+			extended->monitor_timer_interval =
+				DEFAULT_UST_UID_CHANNEL_MONITOR_TIMER;
+			extended->blocking_timeout =
+				DEFAULT_UST_UID_CHANNEL_BLOCKING_TIMEOUT;
+			break;
+		case LTTNG_BUFFER_PER_PID:
+		default:
+			extended->monitor_timer_interval =
+				DEFAULT_UST_PID_CHANNEL_MONITOR_TIMER;
+			extended->blocking_timeout =
+				DEFAULT_UST_PID_CHANNEL_BLOCKING_TIMEOUT;
+			break;
+		}
+	default:
+		/* Default behavior: leave set to 0. */
+		break;
+	}
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_channel_extended_serialize(struct lttng_channel_extended_serialized *dst,
+		const struct lttng_channel_extended *src)
+{
+	assert(src && dst);
+	dst->discarded_events = src->discarded_events;
+	dst->lost_packets = src->lost_packets;
+	dst->monitor_timer_interval = src->monitor_timer_interval;
+	dst->blocking_timeout = src->blocking_timeout;
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_channel_extended_deserialize(struct lttng_channel_extended *dst,
+		const struct lttng_channel_extended_serialized *src)
+{
+	assert(src && dst);
+	dst->discarded_events = src->discarded_events;
+	dst->lost_packets = src->lost_packets;
+	dst->monitor_timer_interval = src->monitor_timer_interval;
+	dst->blocking_timeout = src->blocking_timeout;
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_channel_serialize(struct lttng_channel_serialized *dst,
+		const struct lttng_channel *src)
+{
+	assert(src && dst);
+	struct lttng_channel_attr_serialized *dst_attr = &dst->attr;
+	const struct lttng_channel_attr *src_attr = &src->attr;
+
+	dst_attr->overwrite = src_attr->overwrite;
+	dst_attr->subbuf_size = src_attr->subbuf_size;
+	dst_attr->num_subbuf = src_attr->num_subbuf;
+	dst_attr->switch_timer_interval = src_attr->switch_timer_interval;
+	dst_attr->read_timer_interval = src_attr->read_timer_interval;
+	dst_attr->output = (uint32_t) src_attr->output;
+	dst_attr->tracefile_size = src_attr->tracefile_size;
+	dst_attr->tracefile_count = src_attr->tracefile_count;
+	dst_attr->live_timer_interval = src_attr->live_timer_interval;
+
+	dst->enabled = src->enabled;
+	memcpy(dst->name, src->name, sizeof(dst->name));
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_channel_deserialize(struct lttng_channel *dst,
+		const struct lttng_channel_serialized *src)
+{
+	assert(src && dst);
+	struct lttng_channel_attr *dst_attr = &dst->attr;
+	const struct lttng_channel_attr_serialized *src_attr = &src->attr;
+
+	dst_attr->overwrite = src_attr->overwrite;
+	dst_attr->subbuf_size = src_attr->subbuf_size;
+	dst_attr->num_subbuf = src_attr->num_subbuf;
+	dst_attr->switch_timer_interval = src_attr->switch_timer_interval;
+	dst_attr->read_timer_interval = src_attr->read_timer_interval;
+	dst_attr->output = (enum lttng_event_output) src_attr->output;
+	dst_attr->tracefile_size = src_attr->tracefile_size;
+	dst_attr->tracefile_count = src_attr->tracefile_count;
+	dst_attr->live_timer_interval = src_attr->live_timer_interval;
+
+	dst->enabled = src->enabled;
+	memcpy(dst->name, src->name, sizeof(dst->name));
+	return 0;
+}
+
 LTTNG_HIDDEN
 int sockaddr_in_serialize(struct sockaddr_in_serialized *dst,
 		const struct sockaddr_in *src)
-- 
2.11.0

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

[Yannick Lamarre]

lttng-ctl and lttng-sessiond use serialized communication for
messages using lttng_channel.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/bin/lttng-sessiond/client.c          |  9 ++++++---
 src/common/sessiond-comm/sessiond-comm.h |  4 ++--
 src/lib/lttng-ctl/lttng-ctl.c            | 24 ++++--------------------
 3 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c
index a889529a..0e36e5ad 100644
--- a/src/bin/lttng-sessiond/client.c
+++ b/src/bin/lttng-sessiond/client.c
@@ -1182,10 +1182,13 @@ error_add_context:
 	}
 	case LTTNG_ENABLE_CHANNEL:
 	{
-		cmd_ctx->lsm->u.channel.chan.attr.extended.ptr =
-				(struct lttng_channel_extended *) &cmd_ctx->lsm->u.channel.extended;
+		struct lttng_channel channel;
+		struct lttng_channel_extended extended;
+		lttng_channel_extended_deserialize(&extended, &cmd_ctx->lsm->u.channel.extended);
+		lttng_channel_deserialize(&channel, &cmd_ctx->lsm->u.channel.chan);
+		channel.attr.extended.ptr = &extended;
 		ret = cmd_enable_channel(cmd_ctx->session, &cmd_ctx->lsm->domain,
-				&cmd_ctx->lsm->u.channel.chan,
+				&channel,
 				kernel_poll_pipe[1]);
 		break;
 	}
diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index 23e82c8a..d0905f9e 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -402,9 +402,9 @@ struct lttcomm_session_msg {
 		} LTTNG_PACKED disable;
 		/* Create channel */
 		struct {
-			struct lttng_channel chan LTTNG_PACKED;
+			struct lttng_channel_serialized chan;
 			/* struct lttng_channel_extended is already packed. */
-			struct lttng_channel_extended extended;
+			struct lttng_channel_extended_serialized extended;
 		} LTTNG_PACKED channel;
 		/* Context */
 		struct {
diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
index 165fef4d..789534c8 100644
--- a/src/lib/lttng-ctl/lttng-ctl.c
+++ b/src/lib/lttng-ctl/lttng-ctl.c
@@ -1527,34 +1527,18 @@ int lttng_enable_channel(struct lttng_handle *handle,
 	}
 
 	memset(&lsm, 0, sizeof(lsm));
-	memcpy(&lsm.u.channel.chan, in_chan, sizeof(lsm.u.channel.chan));
-	lsm.u.channel.chan.attr.extended.ptr = NULL;
-
 	if (!in_chan->attr.extended.ptr) {
-		struct lttng_channel *channel;
-		struct lttng_channel_extended *extended;
-
-		channel = lttng_channel_create(&handle->domain);
-		if (!channel) {
-			return -LTTNG_ERR_NOMEM;
-		}
-
-		/*
-		 * Create a new channel in order to use default extended
-		 * attribute values.
-		 */
-		extended = (struct lttng_channel_extended *)
-				channel->attr.extended.ptr;
-		memcpy(&lsm.u.channel.extended, extended, sizeof(*extended));
-		lttng_channel_destroy(channel);
+		init_serialized_extended_channel(&handle->domain, &lsm.u.channel.extended);
 	} else {
 		struct lttng_channel_extended *extended;
 
 		extended = (struct lttng_channel_extended *)
 				in_chan->attr.extended.ptr;
-		memcpy(&lsm.u.channel.extended, extended, sizeof(*extended));
+		lttng_channel_extended_serialize(&lsm.u.channel.extended, extended);
 	}
 
+	lttng_channel_serialize(&lsm.u.channel.chan, in_chan);
+
 	/*
 	 * Verify that the amount of memory required to create the requested
 	 * buffer is available on the system at the moment.
-- 
2.11.0

[RFC PATCH lttng-tools 10/18] Add serialized versions of lttng_event_context structs

[Yannick Lamarre]

Serialized versions of lttng_event_context and lttng_event_perf_counter_ctx are
packed structures to be used in communication protocols for consistent
sizes across platforms. The serialized versions are stripped of pointers
and padding.

Pointers are removed since their size can vary on platforms supporting
variable sized registers (x86-64).
Padding is also removed since it defeats the purpose of a packed struct.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/event-internal.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/lttng/event-internal.h b/include/lttng/event-internal.h
index f8130e3b..25a88448 100644
--- a/include/lttng/event-internal.h
+++ b/include/lttng/event-internal.h
@@ -27,6 +27,17 @@
 
 struct lttng_userspace_probe_location;
 
+struct lttng_event_perf_counter_ctx_serialized {
+	uint32_t type;
+	uint64_t config;
+	char name[LTTNG_SYMBOL_NAME_LEN];
+} LTTNG_PACKED;
+
+struct lttng_event_context_serialized {
+	uint32_t ctx; /* enum lttng_event_context_type */
+	struct lttng_event_perf_counter_ctx_serialized perf_counter;
+} LTTNG_PACKED;
+
 struct lttng_event_extended {
 	/*
 	 * exclusions and filter_expression are only set when the lttng_event
-- 
2.11.0

[RFC PATCH lttng-tools 11/18] Add serdes functions for lttng_event_context

[Yannick Lamarre]

Since those structs are only transfered across unix sockets, endianess
is kept in host order.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/sessiond-comm/sessiond-comm.c | 42 ++++++++++++++++++++++++++++++++
 src/common/sessiond-comm/sessiond-comm.h |  1 +
 2 files changed, 43 insertions(+)

diff --git a/src/common/sessiond-comm/sessiond-comm.c b/src/common/sessiond-comm/sessiond-comm.c
index 6dc58739..84beb90d 100644
--- a/src/common/sessiond-comm/sessiond-comm.c
+++ b/src/common/sessiond-comm/sessiond-comm.c
@@ -76,6 +76,48 @@ static const char *lttcomm_readable_code[] = {
 
 static unsigned long network_timeout;
 
+LTTNG_HIDDEN
+int lttng_event_perf_counter_ctx_serialize(struct lttng_event_perf_counter_ctx_serialized *dst,
+		const struct lttng_event_perf_counter_ctx *src)
+{
+	assert(src && dst);
+	dst->type = src->type;
+	dst->config = src->config;
+	memcpy(&dst->name, &src->name, LTTNG_SYMBOL_NAME_LEN);
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_perf_counter_ctx_deserialize(struct lttng_event_perf_counter_ctx *dst,
+		const struct lttng_event_perf_counter_ctx_serialized *src)
+{
+	assert(src && dst);
+	dst->type = src->type;
+	dst->config = src->config;
+	memcpy(&dst->name, &src->name, LTTNG_SYMBOL_NAME_LEN);
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_context_serialize(struct lttng_event_context_serialized *dst,
+		const struct lttng_event_context *src)
+{
+	assert(src && dst);
+	dst->ctx = (uint32_t) src->ctx;
+	lttng_event_perf_counter_ctx_serialize(&dst->perf_counter, &src->u.perf_counter);
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_context_deserialize(struct lttng_event_context *dst,
+		const struct lttng_event_context_serialized *src)
+{
+	assert(src && dst);
+	dst->ctx = (enum lttng_event_context_type) src->ctx;
+	lttng_event_perf_counter_ctx_deserialize(&dst->u.perf_counter, &src->perf_counter);
+	return 0;
+}
+
 int init_serialized_extended_channel(struct lttng_domain *domain, struct
 		lttng_channel_extended_serialized *extended)
 {
diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index d0905f9e..2760af3a 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -31,6 +31,7 @@
 #include <lttng/snapshot-internal.h>
 #include <lttng/save-internal.h>
 #include <lttng/channel-internal.h>
+#include <lttng/event-internal.h>
 #include <lttng/trigger/trigger-internal.h>
 #include <lttng/rotate-internal.h>
 #include <common/compat/socket.h>
-- 
2.11.0

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

[Yannick Lamarre]

lttng-ctl and lttng-sessiond use serialized communication for
messages using lttng_event_context.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/event-internal.h           |  4 ++++
 src/bin/lttng-sessiond/client.c          | 16 +++++++++-------
 src/common/sessiond-comm/sessiond-comm.h |  2 +-
 src/lib/lttng-ctl/lttng-ctl.c            | 12 +-----------
 4 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/include/lttng/event-internal.h b/include/lttng/event-internal.h
index 25a88448..09b4d232 100644
--- a/include/lttng/event-internal.h
+++ b/include/lttng/event-internal.h
@@ -57,4 +57,8 @@ struct lttng_event_extended {
 LTTNG_HIDDEN
 struct lttng_event *lttng_event_copy(const struct lttng_event *event);
 
+int lttng_event_context_serialize(struct lttng_event_context_serialized *dst, const struct lttng_event_context *src);
+int lttng_event_context_deserialize(struct lttng_event_context *dst, const struct lttng_event_context_serialized *src);
+int lttng_event_perf_counter_ctx_serialize(struct lttng_event_perf_counter_ctx_serialized *dst, const struct lttng_event_perf_counter_ctx *src);
+int lttng_event_perf_counter_ctx_deserialize(struct lttng_event_perf_counter_ctx *dst, const struct lttng_event_perf_counter_ctx_serialized *src);
 #endif /* LTTNG_EVENT_INTERNAL_H */
diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c
index 0e36e5ad..24e688c5 100644
--- a/src/bin/lttng-sessiond/client.c
+++ b/src/bin/lttng-sessiond/client.c
@@ -1069,6 +1069,8 @@ skip_domain:
 	switch (cmd_ctx->lsm->cmd_type) {
 	case LTTNG_ADD_CONTEXT:
 	{
+		struct lttng_event_context evt_ctx;
+		lttng_event_context_deserialize(&evt_ctx, &cmd_ctx->lsm->u.context.ctx);
 		/*
 		 * An LTTNG_ADD_CONTEXT command might have a supplementary
 		 * payload if the context being added is an application context.
@@ -1095,7 +1097,7 @@ skip_domain:
 				ret = -LTTNG_ERR_NOMEM;
 				goto error;
 			}
-			cmd_ctx->lsm->u.context.ctx.u.app_ctx.provider_name =
+			evt_ctx.u.app_ctx.provider_name =
 					provider_name;
 
 			context_name = zmalloc(context_name_len + 1);
@@ -1103,7 +1105,7 @@ skip_domain:
 				ret = -LTTNG_ERR_NOMEM;
 				goto error_add_context;
 			}
-			cmd_ctx->lsm->u.context.ctx.u.app_ctx.ctx_name =
+			evt_ctx.u.app_ctx.ctx_name =
 					context_name;
 
 			ret = lttcomm_recv_unix_sock(sock, provider_name,
@@ -1126,14 +1128,14 @@ skip_domain:
 		ret = cmd_add_context(cmd_ctx->session,
 				cmd_ctx->lsm->domain.type,
 				cmd_ctx->lsm->u.context.channel_name,
-				&cmd_ctx->lsm->u.context.ctx,
+				&evt_ctx,
 				kernel_poll_pipe[1]);
 
-		cmd_ctx->lsm->u.context.ctx.u.app_ctx.provider_name = NULL;
-		cmd_ctx->lsm->u.context.ctx.u.app_ctx.ctx_name = NULL;
+		evt_ctx.u.app_ctx.provider_name = NULL;
+		evt_ctx.u.app_ctx.ctx_name = NULL;
 error_add_context:
-		free(cmd_ctx->lsm->u.context.ctx.u.app_ctx.provider_name);
-		free(cmd_ctx->lsm->u.context.ctx.u.app_ctx.ctx_name);
+		free(evt_ctx.u.app_ctx.provider_name);
+		free(evt_ctx.u.app_ctx.ctx_name);
 		if (ret < 0) {
 			goto error;
 		}
diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h
index 2760af3a..78b18185 100644
--- a/src/common/sessiond-comm/sessiond-comm.h
+++ b/src/common/sessiond-comm/sessiond-comm.h
@@ -410,7 +410,7 @@ struct lttcomm_session_msg {
 		/* Context */
 		struct {
 			char channel_name[LTTNG_SYMBOL_NAME_LEN];
-			struct lttng_event_context ctx LTTNG_PACKED;
+			struct lttng_event_context_serialized ctx;
 			uint32_t provider_name_len;
 			uint32_t context_name_len;
 		} LTTNG_PACKED context;
diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
index 789534c8..686a98ef 100644
--- a/src/lib/lttng-ctl/lttng-ctl.c
+++ b/src/lib/lttng-ctl/lttng-ctl.c
@@ -821,17 +821,7 @@ int lttng_add_context(struct lttng_handle *handle,
 		memcpy(buf, provider_name, provider_len);
 		memcpy(buf + provider_len, ctx_name, ctx_len);
 	}
-	memcpy(&lsm.u.context.ctx, ctx, sizeof(struct lttng_event_context));
-
-	if (ctx->ctx == LTTNG_EVENT_CONTEXT_APP_CONTEXT) {
-		/*
-		 * Don't leak application addresses to the sessiond.
-		 * This is only necessary when ctx is for an app ctx otherwise
-		 * the values inside the union (type & config) are overwritten.
-		 */
-		lsm.u.context.ctx.u.app_ctx.provider_name = NULL;
-		lsm.u.context.ctx.u.app_ctx.ctx_name = NULL;
-	}
+	lttng_event_context_serialize(&lsm.u.context.ctx, ctx);
 
 	ret = lttng_ctl_ask_sessiond_varlen_no_cmd_header(&lsm, buf, len, NULL);
 end:
-- 
2.11.0

[RFC PATCH lttng-tools 13/18] Add serialized versions of lttng_event structs

[Yannick Lamarre]

Serialized versions of lttng_event, lttng_event_function_attr and
lttng_event_probe_attr are packed structures to be used in communication
protocols for consistent sizes across platforms. The serialized versions are
stripped of pointers and padding.

Pointers are removed since their size can vary on platforms supporting
variable sized registers (x86-64).
Padding is also removed since it defeats the purpose of a packed struct.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/event-internal.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/lttng/event-internal.h b/include/lttng/event-internal.h
index 09b4d232..a43007de 100644
--- a/include/lttng/event-internal.h
+++ b/include/lttng/event-internal.h
@@ -27,6 +27,44 @@
 
 struct lttng_userspace_probe_location;
 
+struct lttng_event_probe_attr_serialized {
+	uint64_t addr;
+
+	uint64_t offset;
+	char symbol_name[LTTNG_SYMBOL_NAME_LEN];
+} LTTNG_PACKED;
+
+struct lttng_event_function_attr_serialized {
+	char symbol_name[LTTNG_SYMBOL_NAME_LEN];
+} LTTNG_PACKED;
+
+struct lttng_event_serialized {
+	uint32_t type; /* enum lttng_event_type */
+
+	char name[LTTNG_SYMBOL_NAME_LEN];
+
+	uint32_t loglevel_type; /* enum lttng_loglevel_type */
+
+	int loglevel;
+
+	int32_t enabled;	/* Does not apply: -1 */
+
+	pid_t pid;
+
+	unsigned char filter;	/* filter enabled ? */
+
+	unsigned char exclusion; /* exclusions added ? */
+
+	/* Event flag, from 2.6 and above. */
+	uint32_t flags; /* enum lttng_event_flag */
+
+	/* Per event type configuration */
+	union {
+		struct lttng_event_probe_attr_serialized probe;
+		struct lttng_event_function_attr_serialized ftrace;
+	} attr;
+} LTTNG_PACKED;
+
 struct lttng_event_perf_counter_ctx_serialized {
 	uint32_t type;
 	uint64_t config;
-- 
2.11.0

[RFC PATCH lttng-tools 14/18] Add serdes functions for lttng_event_context

[Yannick Lamarre]

Since those structs are only transfered across unix sockets, endianess
is kept in host order.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/event-internal.h           |   6 ++
 src/common/sessiond-comm/sessiond-comm.c | 116 +++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/include/lttng/event-internal.h b/include/lttng/event-internal.h
index a43007de..ec261af4 100644
--- a/include/lttng/event-internal.h
+++ b/include/lttng/event-internal.h
@@ -95,6 +95,12 @@ struct lttng_event_extended {
 LTTNG_HIDDEN
 struct lttng_event *lttng_event_copy(const struct lttng_event *event);
 
+int lttng_event_probe_attr_serialize(struct lttng_event_serialized *dst, const struct lttng_event *src);
+int lttng_event_function_attr_serialize(struct lttng_event_serialized *dst, const struct lttng_event *src);
+int lttng_event_no_attr_serialize(struct lttng_event_serialized *dst, const struct lttng_event *src);
+int lttng_event_probe_attr_deserialize(struct lttng_event *dst, const struct lttng_event_serialized *src);
+int lttng_event_function_attr_deserialize(struct lttng_event *dst, const struct lttng_event_serialized *src);
+int lttng_event_no_attr_deserialize(struct lttng_event *dst, const struct lttng_event_serialized *src);
 int lttng_event_context_serialize(struct lttng_event_context_serialized *dst, const struct lttng_event_context *src);
 int lttng_event_context_deserialize(struct lttng_event_context *dst, const struct lttng_event_context_serialized *src);
 int lttng_event_perf_counter_ctx_serialize(struct lttng_event_perf_counter_ctx_serialized *dst, const struct lttng_event_perf_counter_ctx *src);
diff --git a/src/common/sessiond-comm/sessiond-comm.c b/src/common/sessiond-comm/sessiond-comm.c
index 84beb90d..d71abe0c 100644
--- a/src/common/sessiond-comm/sessiond-comm.c
+++ b/src/common/sessiond-comm/sessiond-comm.c
@@ -77,6 +77,122 @@ static const char *lttcomm_readable_code[] = {
 static unsigned long network_timeout;
 
 LTTNG_HIDDEN
+int lttng_event_common_serialize(struct lttng_event_serialized *dst,
+		const struct lttng_event *src)
+{
+	dst->type = (uint32_t) src->type;
+
+	memcpy(dst->name, src->name, LTTNG_SYMBOL_NAME_LEN);
+
+	dst->loglevel_type = (uint32_t) src->loglevel_type;
+	dst->loglevel = src->loglevel;
+	dst->enabled = src->enabled;
+	dst->pid = src->pid;
+	dst->filter = src->filter;
+	dst->exclusion = src->exclusion;
+	dst->flags = (uint32_t) src->flags;
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_common_deserialize(struct lttng_event *dst,
+		const struct lttng_event_serialized *src)
+{
+	dst->type = (enum lttng_event_type) src->type;
+
+	memcpy(dst->name, src->name, LTTNG_SYMBOL_NAME_LEN);
+
+	dst->loglevel_type = (enum lttng_loglevel_type) src->loglevel_type;
+	dst->loglevel = src->loglevel;
+	dst->enabled = src->enabled;
+	dst->pid = src->pid;
+	dst->filter = src->filter;
+	dst->exclusion = src->exclusion;
+	dst->flags = (enum lttng_event_flag) src->flags;
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_probe_attr_serialize(struct lttng_event_serialized *dst,
+		const struct lttng_event *src)
+{
+	assert(src && dst);
+	lttng_event_common_serialize(dst, src);
+
+	dst->attr.probe.addr = src->attr.probe.addr;
+	dst->attr.probe.offset = src->attr.probe.offset;
+
+	memcpy(dst->attr.probe.symbol_name, src->attr.probe.symbol_name, LTTNG_SYMBOL_NAME_LEN);
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_probe_attr_deserialize(struct lttng_event *dst,
+		const struct lttng_event_serialized *src)
+{
+	assert(src && dst);
+	lttng_event_common_deserialize(dst, src);
+
+	dst->attr.probe.addr = src->attr.probe.addr;
+	dst->attr.probe.offset = src->attr.probe.offset;
+
+	memcpy(dst->attr.probe.symbol_name, src->attr.probe.symbol_name, LTTNG_SYMBOL_NAME_LEN);
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_function_attr_serialize(struct lttng_event_serialized *dst,
+		const struct lttng_event *src)
+{
+	assert(src && dst);
+	lttng_event_common_serialize(dst, src);
+
+	memcpy(dst->attr.ftrace.symbol_name, src->attr.ftrace.symbol_name, LTTNG_SYMBOL_NAME_LEN);
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_function_attr_deserialize(struct lttng_event *dst,
+		const struct lttng_event_serialized *src)
+{
+	assert(src && dst);
+	lttng_event_common_deserialize(dst, src);
+
+	memcpy(dst->attr.ftrace.symbol_name, src->attr.ftrace.symbol_name, LTTNG_SYMBOL_NAME_LEN);
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_no_attr_serialize(struct lttng_event_serialized *dst,
+		const struct lttng_event *src)
+{
+	assert(src && dst);
+	lttng_event_common_serialize(dst, src);
+
+	memset(&dst->attr, 0, sizeof(dst->attr));
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_event_no_attr_deserialize(struct lttng_event *dst,
+		const struct lttng_event_serialized *src)
+{
+	assert(src && dst);
+	lttng_event_common_deserialize(dst, src);
+
+	memset(&dst->attr, 0, sizeof(dst->attr));
+
+	return 0;
+}
+
+LTTNG_HIDDEN
 int lttng_event_perf_counter_ctx_serialize(struct lttng_event_perf_counter_ctx_serialized *dst,
 		const struct lttng_event_perf_counter_ctx *src)
 {
-- 
2.11.0

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

[Yannick Lamarre]

lttng-ctl and lttng-sessiond use serialized communication for
messages using lttng_event.

Signed-off-by: Yannick Lamarre <ylamarre@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;
 		/*
 		 * 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) {
+			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);
+			}
+		}
 		ret = cmd_disable_event(cmd_ctx->session, cmd_ctx->lsm->domain.type,
 				cmd_ctx->lsm->u.disable.channel_name,
-				&cmd_ctx->lsm->u.disable.event);
+				&event);
 		break;
 	}
 	case LTTNG_ENABLE_CHANNEL:
@@ -1210,6 +1218,7 @@ error_add_context:
 	}
 	case LTTNG_ENABLE_EVENT:
 	{
+		struct lttng_event event;
 		struct lttng_event *ev = NULL;
 		struct lttng_event_exclusion *exclusion = NULL;
 		struct lttng_filter_bytecode *bytecode = NULL;
@@ -1312,7 +1321,15 @@ error_add_context:
 			}
 		}
 
-		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);
+			}
+		}
+		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);
+		}
+	}
 
 	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

[RFC PATCH lttng-tools 16/18] Add serialized versions of lttng_event structs

[Yannick Lamarre]

Serialized version of lttng_snapshot_output is a packed structure to be
used in communication protocols for consistent size across platforms.
The serialized version is stripped of pointers and padding.

Pointers are removed since their size can vary on platforms supporting
variable sized registers (x86-64).
Padding is also removed since it defeats the purpose of a packed struct.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/snapshot-internal.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/lttng/snapshot-internal.h b/include/lttng/snapshot-internal.h
index b7391c22..bf9dfbe4 100644
--- a/include/lttng/snapshot-internal.h
+++ b/include/lttng/snapshot-internal.h
@@ -45,6 +45,14 @@ struct lttng_snapshot_output {
 	char data_url[PATH_MAX];
 };
 
+struct lttng_snapshot_output_serialized {
+	uint32_t id;
+	uint64_t max_size;
+	char name[LTTNG_NAME_MAX];
+	char ctrl_url[PATH_MAX];
+	char data_url[PATH_MAX];
+} LTTNG_PACKED;
+
 /*
  * Snapshot output list object opaque to the user.
  */
-- 
2.11.0

[RFC PATCH lttng-tools 17/18] Add serdes functions for lttng_snapshot_output

[Yannick Lamarre]

Since those structs are only transfered across unix sockets, endianess
is kept in host order.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 include/lttng/snapshot-internal.h        |  3 +++
 src/common/sessiond-comm/sessiond-comm.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/lttng/snapshot-internal.h b/include/lttng/snapshot-internal.h
index bf9dfbe4..b269607d 100644
--- a/include/lttng/snapshot-internal.h
+++ b/include/lttng/snapshot-internal.h
@@ -73,4 +73,7 @@ struct lttng_snapshot_output_list {
 	struct lttng_snapshot_output *array;
 };
 
+int lttng_snapshot_output_serialize(struct lttng_snapshot_output_serialized *dst, const struct lttng_snapshot_output *src);
+int lttng_snapshot_output_deserialize(struct lttng_snapshot_output *dst, const struct lttng_snapshot_output_serialized *src);
+
 #endif /* LTTNG_SNAPSHOT_INTERNAL_ABI_H */
diff --git a/src/common/sessiond-comm/sessiond-comm.c b/src/common/sessiond-comm/sessiond-comm.c
index d71abe0c..863054d0 100644
--- a/src/common/sessiond-comm/sessiond-comm.c
+++ b/src/common/sessiond-comm/sessiond-comm.c
@@ -77,6 +77,34 @@ static const char *lttcomm_readable_code[] = {
 static unsigned long network_timeout;
 
 LTTNG_HIDDEN
+int lttng_snapshot_output_serialize(struct lttng_snapshot_output_serialized *dst,
+		const struct lttng_snapshot_output *src)
+{
+	dst->id = src->id;
+	dst->max_size = src->max_size;
+
+	memcpy(dst->name, src->name, LTTNG_NAME_MAX);
+	memcpy(dst->ctrl_url, src->ctrl_url, PATH_MAX);
+	memcpy(dst->data_url, src->data_url, PATH_MAX);
+
+	return 0;
+}
+
+LTTNG_HIDDEN
+int lttng_snapshot_output_deserialize(struct lttng_snapshot_output *dst,
+		const struct lttng_snapshot_output_serialized *src)
+{
+	dst->id = src->id;
+	dst->max_size = src->max_size;
+
+	memcpy(dst->name, src->name, LTTNG_NAME_MAX);
+	memcpy(dst->ctrl_url, src->ctrl_url, PATH_MAX);
+	memcpy(dst->data_url, src->data_url, PATH_MAX);
+
+	return 0;
+}
+
+LTTNG_HIDDEN
 int lttng_event_common_serialize(struct lttng_event_serialized *dst,
 		const struct lttng_event *src)
 {
-- 
2.11.0

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

[Yannick Lamarre]

lttng-ctl and lttng-sessiond use serialized communication for
messages using lttng_snapshot_out.

Signed-off-by: Yannick Lamarre <ylamarre@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++) {
+			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++) {
+		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

Re: [RFC PATCH lttng-tools 08/18] Add serdes functions for lttng_channel

[Mathieu Desnoyers]

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

> Since those structs are only transfered across unix sockets, endianess

transfered -> transferred

[...]
> 
> +int init_serialized_extended_channel(struct lttng_domain *domain, struct
> +		lttng_channel_extended_serialized *extended)
> +{
> +	assert(domain && extended);
> +	switch (domain->type) {
> +	case LTTNG_DOMAIN_KERNEL:
> +		extended->monitor_timer_interval =
> +			DEFAULT_KERNEL_CHANNEL_MONITOR_TIMER;
> +		extended->blocking_timeout =
> +			DEFAULT_KERNEL_CHANNEL_BLOCKING_TIMEOUT;
> +		break;
> +	case LTTNG_DOMAIN_UST:
> +		switch (domain->buf_type) {
> +		case LTTNG_BUFFER_PER_UID:
> +			extended->monitor_timer_interval =
> +				DEFAULT_UST_UID_CHANNEL_MONITOR_TIMER;
> +			extended->blocking_timeout =
> +				DEFAULT_UST_UID_CHANNEL_BLOCKING_TIMEOUT;
> +			break;
> +		case LTTNG_BUFFER_PER_PID:
> +		default:
> +			extended->monitor_timer_interval =
> +				DEFAULT_UST_PID_CHANNEL_MONITOR_TIMER;
> +			extended->blocking_timeout =
> +				DEFAULT_UST_PID_CHANNEL_BLOCKING_TIMEOUT;
> +			break;
> +		}

missing break here.

> +	default:
> +		/* Default behavior: leave set to 0. */
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
[...]
> +LTTNG_HIDDEN
> +int lttng_channel_serialize(struct lttng_channel_serialized *dst,
> +		const struct lttng_channel *src)
> +{
> +	assert(src && dst);
> +	struct lttng_channel_attr_serialized *dst_attr = &dst->attr;
> +	const struct lttng_channel_attr *src_attr = &src->attr;

in our coding style, all code follows declarations. So here:

struct lttng_channel_attr_serialized *dst_attr;
const struct lttng_channel_attr *src_attr;

assert(src && dst);
dst_attr = &dst->attr;
src_attr = &src->attr;

(same applies elsewhere)

Thanks,

Mathieu
-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 07/18] Add serialized versions of lttng_channel structs

[Mathieu Desnoyers]

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

> Serialized versions of lttng_channel and lttng_channel_extended are
> packed structures to be used in communication protocols for consistent
> sizes across platforms. The serialized versions are stripped of pointers
> and padding.
> 
> Pointers are removed since their size can vary on platforms supporting
> variable sized registers (x86-64).

I don't understand this explanation. I think it's mostly that we have
situations where x86-32 processes interact with x86-64 processes, and
since this is within a communication protocol between processes over
unix socket, it breaks because of pointer size mismatch.

> Padding is also removed since it defeats the purpose of a packed struct.

Not necessarily. Padding was how we extend each command as the protocol evolves.
If we remove padding, how do we plan to extend those in the future ? This commit
documentat how we plan to extend those from now on.

Thanks,

Mathieu


> 
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> include/lttng/channel-internal.h | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
> 
> diff --git a/include/lttng/channel-internal.h b/include/lttng/channel-internal.h
> index 030b4701..c956c19d 100644
> --- a/include/lttng/channel-internal.h
> +++ b/include/lttng/channel-internal.h
> @@ -20,11 +20,41 @@
> 
> #include <common/macros.h>
> 
> +struct lttng_channel_attr_serialized {
> +	int overwrite;                      /* -1: session default, 1: overwrite, 0:
> discard */
> +	uint64_t subbuf_size;               /* bytes, power of 2 */
> +	uint64_t num_subbuf;                /* power of 2 */
> +	unsigned int switch_timer_interval; /* usec */
> +	unsigned int read_timer_interval;   /* usec */
> +	uint32_t output; /* enum lttng_event_output */
> +	/* LTTng 2.1 padding limit */
> +	uint64_t tracefile_size;            /* bytes */
> +	uint64_t tracefile_count;           /* number of tracefiles */
> +	/* LTTng 2.3 padding limit */
> +	unsigned int live_timer_interval;   /* usec */
> +	/* LTTng 2.7 padding limit */
> +
> +} LTTNG_PACKED;
> +
> +struct lttng_channel_serialized {
> +	char name[LTTNG_SYMBOL_NAME_LEN];
> +	uint32_t enabled;
> +	struct lttng_channel_attr_serialized attr;
> +
> +} LTTNG_PACKED;
> +
> struct lttng_channel_extended {
> 	uint64_t discarded_events;
> 	uint64_t lost_packets;
> 	uint64_t monitor_timer_interval;
> 	int64_t blocking_timeout;
> +};
> +
> +struct lttng_channel_extended_serialized {
> +	uint64_t discarded_events;
> +	uint64_t lost_packets;
> +	uint64_t monitor_timer_interval;
> +	int64_t blocking_timeout;
> } LTTNG_PACKED;
> 
> #endif /* LTTNG_CHANNEL_INTERNAL_H */
> --
> 2.11.0
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 10/18] Add serialized versions of lttng_event_context structs

[Mathieu Desnoyers]

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

> Serialized versions of lttng_event_context and lttng_event_perf_counter_ctx are
> packed structures to be used in communication protocols for consistent
> sizes across platforms. The serialized versions are stripped of pointers
> and padding.
> 
> Pointers are removed since their size can vary on platforms supporting
> variable sized registers (x86-64).

Same question as prior commit about the explanation here.

> Padding is also removed since it defeats the purpose of a packed struct.

Same question as prior commit about padding purpose. How do we intend to
extend this in the future ? We should document this plan.

Thanks,

Mathieu

> 
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> include/lttng/event-internal.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/include/lttng/event-internal.h b/include/lttng/event-internal.h
> index f8130e3b..25a88448 100644
> --- a/include/lttng/event-internal.h
> +++ b/include/lttng/event-internal.h
> @@ -27,6 +27,17 @@
> 
> struct lttng_userspace_probe_location;
> 
> +struct lttng_event_perf_counter_ctx_serialized {
> +	uint32_t type;
> +	uint64_t config;
> +	char name[LTTNG_SYMBOL_NAME_LEN];
> +} LTTNG_PACKED;
> +
> +struct lttng_event_context_serialized {
> +	uint32_t ctx; /* enum lttng_event_context_type */
> +	struct lttng_event_perf_counter_ctx_serialized perf_counter;
> +} LTTNG_PACKED;
> +
> struct lttng_event_extended {
> 	/*
> 	 * exclusions and filter_expression are only set when the lttng_event
> --
> 2.11.0
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 11/18] Add serdes functions for lttng_event_context

[Mathieu Desnoyers]

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

> Since those structs are only transfered across unix sockets, endianess

endianness

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 12/18] Integrate serialized communication in lttng-ctl and sessiond

[Mathieu Desnoyers]

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

[...]

> diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c
> index 0e36e5ad..24e688c5 100644
> --- a/src/bin/lttng-sessiond/client.c
> +++ b/src/bin/lttng-sessiond/client.c
> @@ -1069,6 +1069,8 @@ skip_domain:
> 	switch (cmd_ctx->lsm->cmd_type) {
> 	case LTTNG_ADD_CONTEXT:
> 	{
> +		struct lttng_event_context evt_ctx;

coding style: add newline between variable definitions and code.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 13/18] Add serialized versions of lttng_event structs

[Mathieu Desnoyers]

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

> Serialized versions of lttng_event, lttng_event_function_attr and
> lttng_event_probe_attr are packed structures to be used in communication
> protocols for consistent sizes across platforms. The serialized versions are
> stripped of pointers and padding.
> 
> Pointers are removed since their size can vary on platforms supporting
> variable sized registers (x86-64).

Same question about explanation as prior commits.

> Padding is also removed since it defeats the purpose of a packed struct.

Same question about documenting how the protocol is expected to be extended
as prior commits.

Thanks,

Mathieu

> 
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> include/lttng/event-internal.h | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
> 
> diff --git a/include/lttng/event-internal.h b/include/lttng/event-internal.h
> index 09b4d232..a43007de 100644
> --- a/include/lttng/event-internal.h
> +++ b/include/lttng/event-internal.h
> @@ -27,6 +27,44 @@
> 
> struct lttng_userspace_probe_location;
> 
> +struct lttng_event_probe_attr_serialized {
> +	uint64_t addr;
> +
> +	uint64_t offset;
> +	char symbol_name[LTTNG_SYMBOL_NAME_LEN];
> +} LTTNG_PACKED;
> +
> +struct lttng_event_function_attr_serialized {
> +	char symbol_name[LTTNG_SYMBOL_NAME_LEN];
> +} LTTNG_PACKED;
> +
> +struct lttng_event_serialized {
> +	uint32_t type; /* enum lttng_event_type */
> +
> +	char name[LTTNG_SYMBOL_NAME_LEN];
> +
> +	uint32_t loglevel_type; /* enum lttng_loglevel_type */
> +
> +	int loglevel;
> +
> +	int32_t enabled;	/* Does not apply: -1 */
> +
> +	pid_t pid;
> +
> +	unsigned char filter;	/* filter enabled ? */
> +
> +	unsigned char exclusion; /* exclusions added ? */
> +
> +	/* Event flag, from 2.6 and above. */
> +	uint32_t flags; /* enum lttng_event_flag */
> +
> +	/* Per event type configuration */
> +	union {
> +		struct lttng_event_probe_attr_serialized probe;
> +		struct lttng_event_function_attr_serialized ftrace;
> +	} attr;
> +} LTTNG_PACKED;
> +
> struct lttng_event_perf_counter_ctx_serialized {
> 	uint32_t type;
> 	uint64_t config;
> --
> 2.11.0
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 14/18] Add serdes functions for lttng_event_context

[Mathieu Desnoyers]

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

> Since those structs are only transfered across unix sockets, endianess
> is kept in host order.

endianess -> endianness (check in all commits).

Thanks,

Mathieu 


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [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@efficios.com wrote:

> lttng-ctl and lttng-sessiond use serialized communication for
> messages using lttng_event.
> 
> Signed-off-by: Yannick Lamarre <ylamarre@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@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 16/18] Add serialized versions of lttng_event structs

[Mathieu Desnoyers]

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

> Serialized version of lttng_snapshot_output is a packed structure to be
> used in communication protocols for consistent size across platforms.
> The serialized version is stripped of pointers and padding.
> 
> Pointers are removed since their size can vary on platforms supporting
> variable sized registers (x86-64).
> Padding is also removed since it defeats the purpose of a packed struct.

Same questions about explanation of "variable sized registers" and how
we extend without padding as prior commits.

Thanks,

Mathieu

> 
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> include/lttng/snapshot-internal.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/include/lttng/snapshot-internal.h
> b/include/lttng/snapshot-internal.h
> index b7391c22..bf9dfbe4 100644
> --- a/include/lttng/snapshot-internal.h
> +++ b/include/lttng/snapshot-internal.h
> @@ -45,6 +45,14 @@ struct lttng_snapshot_output {
> 	char data_url[PATH_MAX];
> };
> 
> +struct lttng_snapshot_output_serialized {
> +	uint32_t id;
> +	uint64_t max_size;
> +	char name[LTTNG_NAME_MAX];
> +	char ctrl_url[PATH_MAX];
> +	char data_url[PATH_MAX];
> +} LTTNG_PACKED;
> +
> /*
>  * Snapshot output list object opaque to the user.
>  */
> --
> 2.11.0
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH lttng-tools 17/18] Add serdes functions for lttng_snapshot_output

[Mathieu Desnoyers]

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

> Since those structs are only transfered across unix sockets, endianess
> is kept in host order.

transfered -> transferred (check all commit messages)
endianess -> endianness (check all commit messages)

> 
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> include/lttng/snapshot-internal.h        |  3 +++
> src/common/sessiond-comm/sessiond-comm.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 31 insertions(+)
> 
> diff --git a/include/lttng/snapshot-internal.h
> b/include/lttng/snapshot-internal.h
> index bf9dfbe4..b269607d 100644
> --- a/include/lttng/snapshot-internal.h
> +++ b/include/lttng/snapshot-internal.h
> @@ -73,4 +73,7 @@ struct lttng_snapshot_output_list {
> 	struct lttng_snapshot_output *array;
> };
> 
> +int lttng_snapshot_output_serialize(struct lttng_snapshot_output_serialized
> *dst, const struct lttng_snapshot_output *src);
> +int lttng_snapshot_output_deserialize(struct lttng_snapshot_output *dst, const
> struct lttng_snapshot_output_serialized *src);
> +
> #endif /* LTTNG_SNAPSHOT_INTERNAL_ABI_H */
> diff --git a/src/common/sessiond-comm/sessiond-comm.c
> b/src/common/sessiond-comm/sessiond-comm.c
> index d71abe0c..863054d0 100644
> --- a/src/common/sessiond-comm/sessiond-comm.c
> +++ b/src/common/sessiond-comm/sessiond-comm.c
> @@ -77,6 +77,34 @@ static const char *lttcomm_readable_code[] = {
> static unsigned long network_timeout;
> 
> LTTNG_HIDDEN
> +int lttng_snapshot_output_serialize(struct lttng_snapshot_output_serialized
> *dst,
> +		const struct lttng_snapshot_output *src)
> +{

Why not assert(src && dst) here like other helpers ?

> +	dst->id = src->id;
> +	dst->max_size = src->max_size;
> +
> +	memcpy(dst->name, src->name, LTTNG_NAME_MAX);
> +	memcpy(dst->ctrl_url, src->ctrl_url, PATH_MAX);
> +	memcpy(dst->data_url, src->data_url, PATH_MAX);
> +
> +	return 0;
> +}
> +
> +LTTNG_HIDDEN
> +int lttng_snapshot_output_deserialize(struct lttng_snapshot_output *dst,
> +		const struct lttng_snapshot_output_serialized *src)
> +{

same.

Thanks,

Mathieu

> +	dst->id = src->id;
> +	dst->max_size = src->max_size;
> +
> +	memcpy(dst->name, src->name, LTTNG_NAME_MAX);
> +	memcpy(dst->ctrl_url, src->ctrl_url, PATH_MAX);
> +	memcpy(dst->data_url, src->data_url, PATH_MAX);
> +
> +	return 0;
> +}
> +
> +LTTNG_HIDDEN
> int lttng_event_common_serialize(struct lttng_event_serialized *dst,
> 		const struct lttng_event *src)
> {
> --
> 2.11.0
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

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

[Mathieu Desnoyers]

----- On Apr 18, 2019, at 12:18 PM, Yannick Lamarre ylamarre@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@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@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com