All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH lttng-tools v2] Add Unit test to poll compatibility layer
       [not found] <20190321150453.7312-1-ylamarre@efficios.com>
@ 2019-03-21 15:04 ` Yannick Lamarre
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Change LTTNG_POLL_GETNB behaviour for poll flavor Yannick Lamarre
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-21 15:04 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 .gitignore                          |   1 +
 tests/unit/Makefile.am              |   9 +-
 tests/unit/test_utils_compat_poll.c | 236 ++++++++++++++++++++++++++++++++++++
 3 files changed, 245 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit/test_utils_compat_poll.c

diff --git a/.gitignore b/.gitignore
index 19276012..b5d2a55a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -73,6 +73,7 @@ TAGS
 /tests/unit/test_session
 /tests/unit/test_uri
 /tests/unit/test_ust_data
+/tests/unit/test_utils_compat_poll
 /tests/unit/test_utils_parse_size_suffix
 /tests/unit/test_utils_parse_time_suffix
 /tests/unit/test_utils_expand_path
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index 5f485f00..bcda6a5c 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -12,6 +12,7 @@ TESTS = test_kernel_data \
 	test_utils_parse_size_suffix \
 	test_utils_parse_time_suffix \
 	test_utils_expand_path \
+	test_utils_compat_poll \
 	test_string_utils \
 	test_notification \
 	ini_config/test_ini_config
@@ -28,7 +29,8 @@ LIBLTTNG_CTL=$(top_builddir)/src/lib/lttng-ctl/liblttng-ctl.la
 # Define test programs
 noinst_PROGRAMS = test_uri test_session test_kernel_data \
                   test_utils_parse_size_suffix test_utils_parse_time_suffix \
-                  test_utils_expand_path test_string_utils test_notification
+                  test_utils_expand_path test_utils_compat_poll \
+		  test_string_utils test_notification
 
 if HAVE_LIBLTTNG_UST_CTL
 noinst_PROGRAMS += test_ust_data
@@ -145,6 +147,11 @@ test_utils_parse_size_suffix_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON) $(DL
 test_utils_parse_time_suffix_SOURCES = test_utils_parse_time_suffix.c
 test_utils_parse_time_suffix_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
 
+# compat_poll unit test
+test_utils_compat_poll_SOURCES = test_utils_compat_poll.c
+test_utils_compat_poll_LDADD  = $(LIBTAP) $(LIBHASHTABLE) $(DL_LIBS) \
+		      $(top_builddir)/src/common/compat/libcompat.la $(LIBCOMMON)
+
 # expand_path unit test
 test_utils_expand_path_SOURCES = test_utils_expand_path.c
 test_utils_expand_path_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON) $(DL_LIBS)
diff --git a/tests/unit/test_utils_compat_poll.c b/tests/unit/test_utils_compat_poll.c
new file mode 100644
index 00000000..f2c49254
--- /dev/null
+++ b/tests/unit/test_utils_compat_poll.c
@@ -0,0 +1,236 @@
+/*
+ * test_utils_compat_poll.c
+ *
+ * Unit tests for the compatibility layer of poll/epoll API.
+ *
+ * Copyright (C) 2019 Yannick Lamarre <ylamarre@efficios.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by as
+ * published by the Free Software Foundation; only version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <assert.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include <tap/tap.h>
+
+#include <common/compat/poll.h>
+#include <common/readwrite.h>
+
+/* Verification without trashing test order in the child process */
+#define childok(e, test, ...) do { \
+	if (!(e)) { \
+		diag(test, ## __VA_ARGS__); \
+		_exit(EXIT_FAILURE); \
+	} \
+} while(0)
+
+/* For error.h */
+int lttng_opt_quiet = 1;
+int lttng_opt_verbose;
+int lttng_opt_mi;
+
+#ifdef HAVE_EPOLL
+#define NUM_TESTS 46
+#else
+#define NUM_TESTS 45
+#endif
+
+#ifdef HAVE_EPOLL
+#if defined(HAVE_EPOLL_CREATE1) && defined(EPOLL_CLOEXEC)
+#define CLOE_VALUE EPOLL_CLOEXEC
+#else
+#define CLOE_VALUE FD_CLOEXEC
+#endif
+
+/* Non-zero 8-bits arbitrary value below 0x7f to ensure no sign extension
+ * used to verify that the value is properly propagated throught the pipe.
+ */
+#define MAGIC_VALUE ((char)0x5A)
+
+void test_epoll_compat(void)
+{
+	/*
+	 * Type conversion present to disable warning of anonymous enum from
+	 * compiler.
+	 */
+	ok((int)LTTNG_CLOEXEC == (int)CLOE_VALUE, "epoll's CLOEXEC value");
+}
+#endif
+
+void test_alloc(void)
+{
+	struct lttng_poll_event poll_events;
+
+	lttng_poll_init(&poll_events);
+
+	/* Null pointer */
+	ok(lttng_poll_create(NULL, 1, 0) != 0, "Create over NULL pointer fails");
+	/* Size 0 */
+	ok(lttng_poll_create(&poll_events, 0, 0) != 0, "Create with size 0 fails");
+	/* without CLOEXEC */
+	ok(lttng_poll_create(&poll_events, 1, 0) == 0, "Create valid poll set succeeds");
+	/*
+	 * lttng_poll_event structure untested due to incompatibility across
+	 * sublayers. lttng_poll_clean cannot be tested. There is no success
+	 * criteria. Verify set's max size cases.
+	 */
+	lttng_poll_clean(&poll_events);
+}
+
+/* Tests stuff related to what would be handled with epoll_ctl. */
+void test_add_del(void)
+{
+	struct lttng_poll_event poll_events;
+
+	lttng_poll_init(&poll_events);
+	ok(lttng_poll_add(NULL, 1, LPOLLIN) != 0, "Adding to NULL set fails");
+	ok(lttng_poll_add(&poll_events, 1, LPOLLIN) != 0, "Adding to uninitialized structure fails");
+	ok(lttng_poll_add(&poll_events, -1, LPOLLIN) != 0, "Adding invalid FD fails");
+
+	lttng_poll_create(&poll_events, 1, 0);
+	ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set created empty");
+
+	ok(lttng_poll_add(NULL, 1, LPOLLIN) != 0, "Adding to NULL set fails");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set still empty");
+	ok(lttng_poll_add(&poll_events, -1, LPOLLIN) != 0, "Adding invalid FD fails");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set still empty");
+
+	ok(lttng_poll_add(&poll_events, 1, LPOLLIN) == 0, "Adding valid FD succeeds");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Nb of elements incremented");
+
+	ok(lttng_poll_del(NULL, 1) != 0, "Removing from NULL set fails");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of FD in set unchanged");
+
+	ok(lttng_poll_del(&poll_events, -1) != 0, "Removing from negative FD fails");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of FD in set unchanged");
+
+	ok(lttng_poll_del(&poll_events, 2) == 0, "Removing invalid FD still succeeds");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of elements unchanged");
+
+	ok(lttng_poll_del(&poll_events, 1) == 0, "Removing valid FD succeeds");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Nb of elements decremented");
+
+	ok(lttng_poll_del(&poll_events, 1) != 0, "Removing from empty set fails");
+	ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Nb of elements unchanged");
+
+	lttng_poll_clean(&poll_events);
+}
+
+void test_mod_wait(void)
+{
+	struct lttng_poll_event poll_events;
+	struct lttng_poll_event cpoll_events;
+
+	int hupfd[2];
+	int infd[2];
+	pid_t cpid;
+	char rbuf = 0, tbuf = MAGIC_VALUE;
+	int wstatus;
+
+	lttng_poll_init(&poll_events);
+	lttng_poll_init(&cpoll_events);
+
+	ok(pipe(hupfd) != -1);
+	ok(pipe(infd) != -1);
+
+	cpid = fork();
+	if (cpid == 0) {
+		childok(lttng_poll_create(&cpoll_events, 1, 0) == 0, "Create valid poll set succeeds");
+		childok(lttng_poll_mod(NULL, infd[0], LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
+		childok(lttng_poll_mod(&cpoll_events, infd[0], LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
+		childok(lttng_poll_add(&cpoll_events, infd[0], LPOLLHUP) == 0, "Add valid FD succeeds");
+		childok(lttng_poll_mod(&cpoll_events, -1, LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
+		childok(lttng_poll_mod(&cpoll_events, hupfd[0], LPOLLIN) == 0, "lttng_poll_mod on unincluded FD goes on");
+		childok(lttng_poll_mod(&cpoll_events, infd[0], LPOLLIN) == 0, "Modify event type succeeds");
+		childok(close(infd[1]) == 0, "Close valid FD succeeds");
+		childok(lttng_poll_wait(&cpoll_events, -1) == 1, "Wait on close times out");
+		childok(lttng_read(infd[0], &rbuf, 1) == 1, "Data is present in the pipe");
+		childok(rbuf == MAGIC_VALUE, "Received data is consistent with transmitted data");
+		childok(lttng_poll_del(&cpoll_events, infd[0]) == 0, "Removing valid FD succeeds");
+		childok(close(infd[0]) == 0, "Close valid FD succeeds");
+		childok(close(hupfd[0]) == 0, "Close valid FD succeeds");
+		childok(close(hupfd[1]) == 0, "Close valid FD succeeds");
+		lttng_poll_clean(&cpoll_events);
+		_exit(EXIT_SUCCESS);
+	} else {
+		ok(close(hupfd[1]) == 0, "Close valid FD succeeds");
+		ok(close(infd[0]) == 0, "Close valid FD succeeds");
+
+		ok(lttng_poll_wait(NULL, -1) == -1, "lttng_poll_wait call with invalid input returns error");
+
+		ok(lttng_poll_create(&poll_events, 1, 0) == 0, "Create valid poll set succeeds");
+		ok(lttng_poll_wait(&poll_events, -1) == -1, "lttng_poll_wait call with invalid input returns error");
+		ok(lttng_poll_add(&poll_events, hupfd[0], LPOLLHUP) == 0, "Add valid FD succeeds");
+		ok(lttng_write(infd[1], &tbuf, 1) == 1, "Write to pipe succeeds");
+		ok(lttng_poll_wait(&poll_events, -1) == 1, "Wakes up on one event");
+		ok(lttng_poll_del(&poll_events, hupfd[0]) == 0, "Removing valid FD succeeds");
+		ok(close(hupfd[0]) == 0, "Close valid FD succeeds");
+		ok(close(infd[1]) == 0, "Close valid FD succeeds");
+		lttng_poll_clean(&poll_events);
+
+		ok(waitpid(cpid, &wstatus, 0) == cpid, "waitpid returns child's pid");
+		ok(WIFEXITED(wstatus) == 1, "Child process exited");
+		ok(WEXITSTATUS(wstatus) == (EXIT_SUCCESS & 0xff), "EXIT_SUCCESS is %i while exit status is %i\n", EXIT_SUCCESS, WEXITSTATUS(wstatus));
+	}
+
+}
+
+void test_func_def(void)
+{
+#ifdef LTTNG_POLL_GETFD
+#define PASS_GETFD 1
+#else
+#define PASS_GETFD 0
+#endif
+
+#ifdef LTTNG_POLL_GETEV
+#define PASS_GETEV 1
+#else
+#define PASS_GETEV 0
+#endif
+
+#ifdef LTTNG_POLL_GETSZ
+#define PASS_GETSZ 1
+#else
+#define PASS_GETSZ 0
+#endif
+
+#ifdef LTTNG_POLL_GET_PREV_FD
+#define PASS_GET_PREV_FD 1
+#else
+#define PASS_GET_PREV_FD 0
+#endif
+
+	ok(lttng_poll_reset == lttng_poll_reset, "lttng_poll_reset is defined");
+	ok(lttng_poll_init == lttng_poll_init , "lttng_poll_reset is defined");
+	ok(PASS_GETFD, "GETFD is defined");
+	ok(PASS_GETEV, "GETEV is defined");
+	ok(PASS_GETSZ, "GETSZ is defined");
+	ok(PASS_GET_PREV_FD, "GET_PREV_FD is defined");
+
+}
+
+int main(void)
+{
+	plan_tests(NUM_TESTS);
+#ifdef HAVE_EPOLL
+	test_epoll_compat();
+#endif
+	test_func_def();
+	test_alloc();
+	test_add_del();
+	test_mod_wait();
+	return exit_status();
+}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH lttng-tools v2] Change LTTNG_POLL_GETNB behaviour for poll flavor
       [not found] <20190321150453.7312-1-ylamarre@efficios.com>
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Add Unit test to poll compatibility layer Yannick Lamarre
@ 2019-03-21 15:04 ` Yannick Lamarre
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Adapt poll layer behaviour to match the epoll layer Yannick Lamarre
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-21 15:04 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

Modify LTTNG_POLL_GETNB to provide compatibility with the epoll flavor.
Since it is only used after a lttng_poll_wait call with no modification
(add, del, mod) between, this change does not modify the behaviour in
its current usage while ensuring similar API behavior between
compatibility layer implementations.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/compat/poll.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index fde54ddb..5eb7ff9c 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -315,10 +315,12 @@ static inline int __lttng_poll_get_prev_fd(struct lttng_poll_event *events,
 /*
  * For the following calls, consider 'e' to be a lttng_poll_event pointer and i
  * being the index of the events array.
+ * LTTNG_POLL_GETNB is always used after lttng_poll_wait, thus we can use the
+ * current list for test compatibility purposes.
  */
 #define LTTNG_POLL_GETFD(e, i) LTTNG_REF(e)->wait.events[i].fd
 #define LTTNG_POLL_GETEV(e, i) LTTNG_REF(e)->wait.events[i].revents
-#define LTTNG_POLL_GETNB(e) LTTNG_REF(e)->wait.nb_fd
+#define LTTNG_POLL_GETNB(e) LTTNG_REF(e)->current.nb_fd
 #define LTTNG_POLL_GETSZ(e) LTTNG_REF(e)->wait.events_size
 #define LTTNG_POLL_GET_PREV_FD(e, i, nb_fd) \
 	__lttng_poll_get_prev_fd(LTTNG_REF(e), i, nb_fd)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH lttng-tools v2] Adapt poll layer behaviour to match the epoll layer
       [not found] <20190321150453.7312-1-ylamarre@efficios.com>
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Add Unit test to poll compatibility layer Yannick Lamarre
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Change LTTNG_POLL_GETNB behaviour for poll flavor Yannick Lamarre
@ 2019-03-21 15:04 ` Yannick Lamarre
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Fix hang in thread_rotation when using compat-poll Yannick Lamarre
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-21 15:04 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/compat/compat-poll.c | 28 ++++++++++++++++++----------
 src/common/compat/poll.h        |  1 +
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
index b45b39dc..b7c17df7 100644
--- a/src/common/compat/compat-poll.c
+++ b/src/common/compat/compat-poll.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (C)  2011 - David Goulet <david.goulet@polymtl.ca>
+ * Copyright (C)  2019 - Yannick Lamarre <ylamarre@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,
@@ -204,10 +205,10 @@ int compat_poll_mod(struct lttng_poll_event *events, int fd,
 		uint32_t req_events)
 {
 	int i;
-	bool fd_found = false;
 	struct compat_poll_event_array *current;
 
-	if (events == NULL || events->current.events == NULL || fd < 0) {
+	if (events == NULL || events->current.nb_fd == 0 || events->current.events
+			== NULL || fd < 0) {
 		ERR("Bad compat poll mod arguments");
 		goto error;
 	}
@@ -216,16 +217,15 @@ int compat_poll_mod(struct lttng_poll_event *events, int fd,
 
 	for (i = 0; i < current->nb_fd; i++) {
 		if (current->events[i].fd == fd) {
-			fd_found = true;
 			current->events[i].events = req_events;
 			events->need_update = 1;
 			break;
 		}
 	}
 
-	if (!fd_found) {
-		goto error;
-	}
+	/* 
+	 * The epoll flavor doesn't flag modifying a non-included FD as an error.
+	 */
 
 	return 0;
 
@@ -238,11 +238,12 @@ error:
  */
 int compat_poll_del(struct lttng_poll_event *events, int fd)
 {
-	int new_size, i, count = 0, ret;
+	int i, count = 0, ret;
+	uint32_t new_size;
 	struct compat_poll_event_array *current;
 
-	if (events == NULL || events->current.events == NULL || fd < 0) {
-		ERR("Wrong arguments for poll del");
+	if (events == NULL || events->current.nb_fd == 0 || events->current.events
+			== NULL || fd < 0) {
 		goto error;
 	}
 
@@ -257,13 +258,19 @@ int compat_poll_del(struct lttng_poll_event *events, int fd)
 			count++;
 		}
 	}
+
+	/* The fd was not in our set, return no error as with epoll. */
+	if (current->nb_fd == count) {
+		goto skip;
+	}
+
 	/* No fd duplicate should be ever added into array. */
 	assert(current->nb_fd - 1 == count);
 	current->nb_fd = count;
 
 	/* Resize array if needed. */
 	new_size = 1U << utils_get_count_order_u32(current->nb_fd);
-	if (new_size != current->alloc_size && new_size >= current->init_size) {
+	if (new_size != current->alloc_size && new_size >= current->init_size && current->nb_fd != 0) {
 		ret = resize_poll_event(current, new_size);
 		if (ret < 0) {
 			goto error;
@@ -272,6 +279,7 @@ int compat_poll_del(struct lttng_poll_event *events, int fd)
 
 	events->need_update = 1;
 
+skip:
 	return 0;
 
 error:
diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index 5eb7ff9c..d2826b7a 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2011 - David Goulet <david.goulet@polymtl.ca>
+ * Copyright (C) 2019 - Yannick Lamarre <ylamarre@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,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH lttng-tools v2] Fix hang in thread_rotation when using compat-poll
       [not found] <20190321150453.7312-1-ylamarre@efficios.com>
                   ` (2 preceding siblings ...)
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Adapt poll layer behaviour to match the epoll layer Yannick Lamarre
@ 2019-03-21 15:04 ` Yannick Lamarre
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Yannick Lamarre
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-21 15:04 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

Add missing verification to prevent a blocking read on an empty fd.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/bin/lttng-sessiond/rotation-thread.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/bin/lttng-sessiond/rotation-thread.c b/src/bin/lttng-sessiond/rotation-thread.c
index 6669372d..b86b1668 100644
--- a/src/bin/lttng-sessiond/rotation-thread.c
+++ b/src/bin/lttng-sessiond/rotation-thread.c
@@ -974,6 +974,10 @@ void *thread_rotation(void *data)
 			int fd = LTTNG_POLL_GETFD(&thread.events, i);
 			uint32_t revents = LTTNG_POLL_GETEV(&thread.events, i);
 
+			if (!revents) {
+				/* No activity for this FD (poll implementation). */
+				continue;
+			}
 			DBG("[rotation-thread] Handling fd (%i) activity (%u)",
 					fd, revents);
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
       [not found] <20190321150453.7312-1-ylamarre@efficios.com>
                   ` (3 preceding siblings ...)
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Fix hang in thread_rotation when using compat-poll Yannick Lamarre
@ 2019-03-21 15:04 ` Yannick Lamarre
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Clean code base from redundant verification Yannick Lamarre
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-21 15:04 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

This removes the need to verify for eventless file descriptors and
mitigates risks of bug due to behaviour mismatch.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/common/compat/compat-poll.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
index b7c17df7..6aa9414d 100644
--- a/src/common/compat/compat-poll.c
+++ b/src/common/compat/compat-poll.c
@@ -292,6 +292,7 @@ error:
 int compat_poll_wait(struct lttng_poll_event *events, int timeout)
 {
 	int ret;
+	int eidx = 0;
 
 	if (events == NULL || events->current.events == NULL) {
 		ERR("poll wait arguments error");
@@ -324,10 +325,36 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
 	}
 
 	/*
-	 * poll() should always iterate on all FDs since we handle the pollset in
-	 * user space and after poll returns, we have to try every fd for a match.
+	 * Swap all nonzero revents pollfd structs to the beginning of the array to
+	 * emulate compat-epoll behaviour. This algorithm takes advantage of poll's
+	 * returned value and the burst nature of waking events on the file
+	 * descriptors. The while loop garantees that idlepfd will always point to
+	 * a pollfd with revents == 0.
 	 */
-	return events->wait.nb_fd;
+	if (ret == events->wait.nb_fd) {
+		goto skip;
+	}
+	while (eidx < ret && events->wait.events[eidx].revents != 0) {
+		eidx += 1;
+	}
+
+	for (int idx = eidx + 1; eidx < ret; idx++) {
+		struct pollfd swapfd;
+		struct pollfd *idlepfd = &events->wait.events[eidx];
+		struct pollfd *ipfd = &events->wait.events[idx];
+
+		assert(idx < events->wait.nb_fd);
+
+		if (ipfd->revents != 0) {
+			swapfd = *ipfd;
+			*ipfd = *idlepfd;
+			*idlepfd = swapfd;
+			eidx += 1;
+		}
+	}
+
+skip:
+	return ret;
 
 error:
 	return -1;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH lttng-tools v2] Clean code base from redundant verification
       [not found] <20190321150453.7312-1-ylamarre@efficios.com>
                   ` (4 preceding siblings ...)
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Yannick Lamarre
@ 2019-03-21 15:04 ` Yannick Lamarre
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Fix typo Yannick Lamarre
       [not found] ` <20190321150453.7312-6-ylamarre@efficios.com>
  7 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-21 15:04 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

Remove redundant verification for file descriptors with 0 revents in
code base.

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 src/bin/lttng-consumerd/health-consumerd.c   |  5 -----
 src/bin/lttng-relayd/health-relayd.c         |  5 -----
 src/bin/lttng-relayd/live.c                  | 10 ----------
 src/bin/lttng-relayd/main.c                  | 16 ----------------
 src/bin/lttng-sessiond/agent-thread.c        |  5 -----
 src/bin/lttng-sessiond/client.c              |  5 -----
 src/bin/lttng-sessiond/dispatch.c            |  5 -----
 src/bin/lttng-sessiond/health.c              |  5 -----
 src/bin/lttng-sessiond/ht-cleanup.c          |  5 -----
 src/bin/lttng-sessiond/manage-apps.c         |  5 -----
 src/bin/lttng-sessiond/manage-consumer.c     | 10 ----------
 src/bin/lttng-sessiond/manage-kernel.c       |  5 -----
 src/bin/lttng-sessiond/notification-thread.c |  3 ---
 src/bin/lttng-sessiond/notify-apps.c         |  5 -----
 src/bin/lttng-sessiond/register.c            |  5 -----
 src/bin/lttng-sessiond/rotation-thread.c     |  4 ----
 src/common/consumer/consumer.c               | 10 ----------
 17 files changed, 108 deletions(-)

diff --git a/src/bin/lttng-consumerd/health-consumerd.c b/src/bin/lttng-consumerd/health-consumerd.c
index 1e2f31e4..7d60e54e 100644
--- a/src/bin/lttng-consumerd/health-consumerd.c
+++ b/src/bin/lttng-consumerd/health-consumerd.c
@@ -259,11 +259,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			ret = check_health_quit_pipe(pollfd, revents);
 			if (ret) {
diff --git a/src/bin/lttng-relayd/health-relayd.c b/src/bin/lttng-relayd/health-relayd.c
index ba996621..b782c3e7 100644
--- a/src/bin/lttng-relayd/health-relayd.c
+++ b/src/bin/lttng-relayd/health-relayd.c
@@ -329,11 +329,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			ret = check_health_quit_pipe(pollfd, revents);
 			if (ret) {
diff --git a/src/bin/lttng-relayd/live.c b/src/bin/lttng-relayd/live.c
index 9efab8a7..9503ba47 100644
--- a/src/bin/lttng-relayd/live.c
+++ b/src/bin/lttng-relayd/live.c
@@ -543,11 +543,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			ret = check_thread_quit_pipe(pollfd, revents);
 			if (ret) {
@@ -2000,11 +1995,6 @@ restart:
 
 			health_code_update();
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			ret = check_thread_quit_pipe(pollfd, revents);
 			if (ret) {
diff --git a/src/bin/lttng-relayd/main.c b/src/bin/lttng-relayd/main.c
index de14cdb4..42f871b5 100644
--- a/src/bin/lttng-relayd/main.c
+++ b/src/bin/lttng-relayd/main.c
@@ -858,14 +858,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/*
-				 * No activity for this FD (poll
-				 * implementation).
-				 */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			ret = check_thread_quit_pipe(pollfd, revents);
 			if (ret) {
@@ -3811,14 +3803,6 @@ restart:
 
 			health_code_update();
 
-			if (!revents) {
-				/*
-				 * No activity for this FD (poll
-				 * implementation).
-				 */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			ret = check_thread_quit_pipe(pollfd, revents);
 			if (ret) {
diff --git a/src/bin/lttng-sessiond/agent-thread.c b/src/bin/lttng-sessiond/agent-thread.c
index f7be1ef7..575f6aee 100644
--- a/src/bin/lttng-sessiond/agent-thread.c
+++ b/src/bin/lttng-sessiond/agent-thread.c
@@ -404,11 +404,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			if (pollfd == quit_pipe_read_fd) {
 				goto exit;
diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c
index a889529a..0be065ec 100644
--- a/src/bin/lttng-sessiond/client.c
+++ b/src/bin/lttng-sessiond/client.c
@@ -2174,11 +2174,6 @@ static void *thread_manage_clients(void *data)
 
 			health_code_update();
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			if (pollfd == thread_quit_pipe_fd) {
 				err = 0;
 				goto exit;
diff --git a/src/bin/lttng-sessiond/dispatch.c b/src/bin/lttng-sessiond/dispatch.c
index 04b3954c..a637f23d 100644
--- a/src/bin/lttng-sessiond/dispatch.c
+++ b/src/bin/lttng-sessiond/dispatch.c
@@ -144,11 +144,6 @@ static void sanitize_wait_queue(struct ust_reg_wait_queue *wait_queue)
 		uint32_t revents = LTTNG_POLL_GETEV(&events, i);
 		int pollfd = LTTNG_POLL_GETFD(&events, i);
 
-		if (!revents) {
-			/* No activity for this FD (poll implementation). */
-			continue;
-		}
-
 		cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
 				&wait_queue->head, head) {
 			if (pollfd == wait_node->app->sock &&
diff --git a/src/bin/lttng-sessiond/health.c b/src/bin/lttng-sessiond/health.c
index 921b4526..5b4f5f8f 100644
--- a/src/bin/lttng-sessiond/health.c
+++ b/src/bin/lttng-sessiond/health.c
@@ -158,11 +158,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Event on the registration socket */
 			if (pollfd == sock) {
 				if (revents & LPOLLIN) {
diff --git a/src/bin/lttng-sessiond/ht-cleanup.c b/src/bin/lttng-sessiond/ht-cleanup.c
index 91c0544c..02244f0c 100644
--- a/src/bin/lttng-sessiond/ht-cleanup.c
+++ b/src/bin/lttng-sessiond/ht-cleanup.c
@@ -155,11 +155,6 @@ static void *thread_ht_cleanup(void *data)
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			if (pollfd != ht_cleanup_pipe[0]) {
 				continue;
 			}
diff --git a/src/bin/lttng-sessiond/manage-apps.c b/src/bin/lttng-sessiond/manage-apps.c
index f9ec356d..356c147a 100644
--- a/src/bin/lttng-sessiond/manage-apps.c
+++ b/src/bin/lttng-sessiond/manage-apps.c
@@ -123,11 +123,6 @@ static void *thread_application_management(void *data)
 
 			health_code_update();
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			if (pollfd == quit_pipe_read_fd) {
 				err = 0;
 				goto exit;
diff --git a/src/bin/lttng-sessiond/manage-consumer.c b/src/bin/lttng-sessiond/manage-consumer.c
index b710c61f..fa802adc 100644
--- a/src/bin/lttng-sessiond/manage-consumer.c
+++ b/src/bin/lttng-sessiond/manage-consumer.c
@@ -134,11 +134,6 @@ void *thread_consumer_management(void *data)
 
 		health_code_update();
 
-		if (!revents) {
-			/* No activity for this FD (poll implementation). */
-			continue;
-		}
-
 		/* Thread quit pipe has been closed. Killing thread. */
 		if (pollfd == quit_pipe_read_fd) {
 			err = 0;
@@ -298,11 +293,6 @@ void *thread_consumer_management(void *data)
 
 			health_code_update();
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/*
 			 * Thread quit pipe has been triggered, flag that we should stop
 			 * but continue the current loop to handle potential data from
diff --git a/src/bin/lttng-sessiond/manage-kernel.c b/src/bin/lttng-sessiond/manage-kernel.c
index 34887d7b..f656c9f5 100644
--- a/src/bin/lttng-sessiond/manage-kernel.c
+++ b/src/bin/lttng-sessiond/manage-kernel.c
@@ -267,11 +267,6 @@ static void *thread_kernel_management(void *data)
 
 			health_code_update();
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			if (pollfd == quit_pipe_read_fd) {
 				err = 0;
 				goto exit;
diff --git a/src/bin/lttng-sessiond/notification-thread.c b/src/bin/lttng-sessiond/notification-thread.c
index b42b282e..0a1fd557 100644
--- a/src/bin/lttng-sessiond/notification-thread.c
+++ b/src/bin/lttng-sessiond/notification-thread.c
@@ -562,9 +562,6 @@ void *thread_notification(void *data)
 			int fd = LTTNG_POLL_GETFD(&state.events, i);
 			uint32_t revents = LTTNG_POLL_GETEV(&state.events, i);
 
-			if (!revents) {
-				continue;
-			}
 			DBG("[notification-thread] Handling fd (%i) activity (%u)", fd, revents);
 
 			if (fd == state.notification_channel_socket) {
diff --git a/src/bin/lttng-sessiond/notify-apps.c b/src/bin/lttng-sessiond/notify-apps.c
index bc7405c7..17f3cd0c 100644
--- a/src/bin/lttng-sessiond/notify-apps.c
+++ b/src/bin/lttng-sessiond/notify-apps.c
@@ -109,11 +109,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			if (pollfd == quit_pipe_read_fd) {
 				err = 0;
diff --git a/src/bin/lttng-sessiond/register.c b/src/bin/lttng-sessiond/register.c
index 6ce25ad0..e809834f 100644
--- a/src/bin/lttng-sessiond/register.c
+++ b/src/bin/lttng-sessiond/register.c
@@ -239,11 +239,6 @@ static void *thread_application_registration(void *data)
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			/* Thread quit pipe has been closed. Killing thread. */
 			if (pollfd == quit_pipe_read_fd) {
 				err = 0;
diff --git a/src/bin/lttng-sessiond/rotation-thread.c b/src/bin/lttng-sessiond/rotation-thread.c
index b86b1668..6669372d 100644
--- a/src/bin/lttng-sessiond/rotation-thread.c
+++ b/src/bin/lttng-sessiond/rotation-thread.c
@@ -974,10 +974,6 @@ void *thread_rotation(void *data)
 			int fd = LTTNG_POLL_GETFD(&thread.events, i);
 			uint32_t revents = LTTNG_POLL_GETEV(&thread.events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
 			DBG("[rotation-thread] Handling fd (%i) activity (%u)",
 					fd, revents);
 
diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index 10273e66..c70d95a5 100644
--- a/src/common/consumer/consumer.c
+++ b/src/common/consumer/consumer.c
@@ -2396,11 +2396,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			if (pollfd == lttng_pipe_get_readfd(ctx->consumer_metadata_pipe)) {
 				if (revents & LPOLLIN) {
 					ssize_t pipe_len;
@@ -2990,11 +2985,6 @@ restart:
 			revents = LTTNG_POLL_GETEV(&events, i);
 			pollfd = LTTNG_POLL_GETFD(&events, i);
 
-			if (!revents) {
-				/* No activity for this FD (poll implementation). */
-				continue;
-			}
-
 			if (pollfd == ctx->consumer_channel_pipe[0]) {
 				if (revents & LPOLLIN) {
 					enum consumer_channel_action action;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH lttng-tools v2] Fix typo
       [not found] <20190321150453.7312-1-ylamarre@efficios.com>
                   ` (5 preceding siblings ...)
  2019-03-21 15:04 ` [PATCH lttng-tools v2] Clean code base from redundant verification Yannick Lamarre
@ 2019-03-21 15:04 ` Yannick Lamarre
       [not found] ` <20190321150453.7312-6-ylamarre@efficios.com>
  7 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-21 15:04 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

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

diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index d2826b7a..c7bd0606 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -359,7 +359,7 @@ extern int compat_poll_del(struct lttng_poll_event *events, int fd);
 	compat_poll_del(events, fd)
 
 /*
- * Modify an fd's events in the epoll set.
+ * Modify an fd's events in the poll set.
  */
 extern int compat_poll_mod(struct lttng_poll_event *events,
 		int fd, uint32_t req_events);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
       [not found] ` <20190321150453.7312-6-ylamarre@efficios.com>
@ 2019-03-21 20:25   ` Jérémie Galarneau
       [not found]   ` <CA+jJMxui0_5EF6XLH-9G90-yJEE63zdiBifTeF4i9iQp95D7tg@mail.gmail.com>
  1 sibling, 0 replies; 12+ messages in thread
From: Jérémie Galarneau @ 2019-03-21 20:25 UTC (permalink / raw)
  To: Yannick Lamarre; +Cc: lttng-dev, Jeremie Galarneau

Thanks for this patch, it does allow a worthwhile clean-up!
Read on for comments about style.

On Thu, 21 Mar 2019 at 11:05, Yannick Lamarre <ylamarre@efficios.com> wrote:
>
> This removes the need to verify for eventless file descriptors and
> mitigates risks of bug due to behaviour mismatch.
>
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
>  src/common/compat/compat-poll.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
> index b7c17df7..6aa9414d 100644
> --- a/src/common/compat/compat-poll.c
> +++ b/src/common/compat/compat-poll.c
> @@ -292,6 +292,7 @@ error:
>  int compat_poll_wait(struct lttng_poll_event *events, int timeout)
>  {
>         int ret;
> +       int eidx = 0;

Reading the code, this seems to mean the "index of the first idle file
descriptor".

As a general rule, don't use uncommon abbreviations. Abbreviations
are okay when they are common-place, unambiguous and save
a lot of characters (e.g. cfg for configuration, or lttng for
linux_trace_toolkit_next_generation).

When in doubt, prefer verbose names.

In this case, "idle_pfd_index" and a comment (see below) would
clarify the intent.

>
>         if (events == NULL || events->current.events == NULL) {
>                 ERR("poll wait arguments error");
> @@ -324,10 +325,36 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
>         }
>
>         /*
> -        * poll() should always iterate on all FDs since we handle the pollset in
> -        * user space and after poll returns, we have to try every fd for a match.
> +        * Swap all nonzero revents pollfd structs to the beginning of the array to
> +        * emulate compat-epoll behaviour. This algorithm takes advantage of poll's
> +        * returned value and the burst nature of waking events on the file
> +        * descriptors. The while loop garantees that idlepfd will always point to

garantees -> guarantees

This comment block exceeds 80 chars. Make sure your editor's tab width is set
to 8 characters.

The code, commit message, and comments make use of multiple terms
to refer to the same file descriptor state ('idle', 'empty',
'eventless', and 'zero').

Please stick to one of those terms.

Idle seems preferable, but feel free to whichever term the poll() specification
uses.

> +        * a pollfd with revents == 0.
>          */
> -       return events->wait.nb_fd;
> +       if (ret == events->wait.nb_fd) {
> +               goto skip;
> +       }
> +       while (eidx < ret && events->wait.events[eidx].revents != 0) {
> +               eidx += 1;

Prefer "eidx++".

Using variables for a single purpose is fine and tends to make the
code clearer.

For example, reworking this section of code to:

active_fd_count = ret;
/* Find the first idle pollfd. */
while (idle_pfd_index < active_fd_count &&
events->wait.events[idle_pfd_index].revents != 0) {
        idle_pfd_index++;
}

makes the intent clearer.

> +       }
> +
> +       for (int idx = eidx + 1; eidx < ret; idx++) {

Same change here (regarding 'ret').

To maintain uniformity with the rest of the code base, please declare
variables at the top of the inner-most scope containing the loop.

When declaring a trivial loop counter/index, use the name 'i'. Here, the
difference between 'idx' and 'eidx' is not immediately apparent.
Renaming 'eidx' makes the loop clearer.

Also, in this particular case, 'i' is fine since you are only using it to
assign the current pollfd, which is fairly descriptive.

> +               struct pollfd swapfd;
> +               struct pollfd *idlepfd = &events->wait.events[eidx];
> +               struct pollfd *ipfd = &events->wait.events[idx];

Use an underscore to split variable names, e.g. swap_pfd, idle_pfd,
current_pfd. Again, verbose names are preferable.

> +
> +               assert(idx < events->wait.nb_fd);

Not needed. Using assert() is fine to highlight assumptions across
interfaces/functions and/or catch catastrophic internal errors. Here,
this would be a very local condition that seems to be already
enforced.

Reworking the 'for' condition to use 'i < events->wait.nb_fd' and
breaking out early of the loop is also an option here.

> +
> +               if (ipfd->revents != 0) {
> +                       swapfd = *ipfd;
> +                       *ipfd = *idlepfd;
> +                       *idlepfd = swapfd;
> +                       eidx += 1;

eidx++;

> +               }
> +       }
> +
> +skip:

Rename label to 'end'.

Thanks!
Jérémie


> +       return ret;
>
>  error:
>         return -1;
> --
> 2.11.0
>


--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
       [not found]   ` <CA+jJMxui0_5EF6XLH-9G90-yJEE63zdiBifTeF4i9iQp95D7tg@mail.gmail.com>
@ 2019-03-22 15:04     ` Mathieu Desnoyers
       [not found]     ` <8703543.6154.1553267076349.JavaMail.zimbra@efficios.com>
  1 sibling, 0 replies; 12+ messages in thread
From: Mathieu Desnoyers @ 2019-03-22 15:04 UTC (permalink / raw)
  To: Jeremie Galarneau; +Cc: lttng-dev, Jeremie Galarneau

----- On Mar 21, 2019, at 4:25 PM, Jeremie Galarneau jeremie.galarneau@efficios.com wrote:

[...]
>> +               assert(idx < events->wait.nb_fd);
> 
> Not needed. Using assert() is fine to highlight assumptions across
> interfaces/functions and/or catch catastrophic internal errors. Here,
> this would be a very local condition that seems to be already
> enforced.
> 
> Reworking the 'for' condition to use 'i < events->wait.nb_fd' and
> breaking out early of the loop is also an option here.

The reason for using assert() here is to ensure that there is no discrepancy
between the number of active poll fd in the set and the value returned by
poll indicating that same number of active fds.

We'd need a kernel bug (or memory corruption) to have a mismatch, but I thought
having an assert in there would not hurt, since it might help diagnose issues
that cross user-kernel boundaries.

Thoughts ?

Mathieu

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
       [not found]     ` <8703543.6154.1553267076349.JavaMail.zimbra@efficios.com>
@ 2019-03-25 15:10       ` Jérémie Galarneau
       [not found]       ` <CA+jJMxsbA0ksOJN9yqbtCVXkTLKOqfEMek6P9_bxSdKH75zEPQ@mail.gmail.com>
  1 sibling, 0 replies; 12+ messages in thread
From: Jérémie Galarneau @ 2019-03-25 15:10 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: lttng-dev, Jeremie Galarneau

On Fri, 22 Mar 2019 at 11:04, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Mar 21, 2019, at 4:25 PM, Jeremie Galarneau jeremie.galarneau@efficios.com wrote:
>
> [...]
> >> +               assert(idx < events->wait.nb_fd);
> >
> > Not needed. Using assert() is fine to highlight assumptions across
> > interfaces/functions and/or catch catastrophic internal errors. Here,
> > this would be a very local condition that seems to be already
> > enforced.
> >
> > Reworking the 'for' condition to use 'i < events->wait.nb_fd' and
> > breaking out early of the loop is also an option here.
>
> The reason for using assert() here is to ensure that there is no discrepancy
> between the number of active poll fd in the set and the value returned by
> poll indicating that same number of active fds.
>
> We'd need a kernel bug (or memory corruption) to have a mismatch, but I thought
> having an assert in there would not hurt, since it might help diagnose issues
> that cross user-kernel boundaries.
>
> Thoughts ?

Hi,

I understand the intent, but I don't feel these type of kernel tests/checks
belong in the code.

However, if there is a specific concern that our use of poll() could highlight
this type of bug (in the kernel or in the interactions with lttng-modules),
we can add tests for that.

Thanks,
Jérémie

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


--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
       [not found]       ` <CA+jJMxsbA0ksOJN9yqbtCVXkTLKOqfEMek6P9_bxSdKH75zEPQ@mail.gmail.com>
@ 2019-03-25 15:56         ` Mathieu Desnoyers
       [not found]         ` <1869646214.7612.1553529371615.JavaMail.zimbra@efficios.com>
  1 sibling, 0 replies; 12+ messages in thread
From: Mathieu Desnoyers @ 2019-03-25 15:56 UTC (permalink / raw)
  To: Jeremie Galarneau; +Cc: lttng-dev, Jeremie Galarneau

----- On Mar 25, 2019, at 11:10 AM, Jeremie Galarneau jeremie.galarneau@efficios.com wrote:

> On Fri, 22 Mar 2019 at 11:04, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> ----- On Mar 21, 2019, at 4:25 PM, Jeremie Galarneau
>> jeremie.galarneau@efficios.com wrote:
>>
>> [...]
>> >> +               assert(idx < events->wait.nb_fd);
>> >
>> > Not needed. Using assert() is fine to highlight assumptions across
>> > interfaces/functions and/or catch catastrophic internal errors. Here,
>> > this would be a very local condition that seems to be already
>> > enforced.
>> >
>> > Reworking the 'for' condition to use 'i < events->wait.nb_fd' and
>> > breaking out early of the loop is also an option here.
>>
>> The reason for using assert() here is to ensure that there is no discrepancy
>> between the number of active poll fd in the set and the value returned by
>> poll indicating that same number of active fds.
>>
>> We'd need a kernel bug (or memory corruption) to have a mismatch, but I thought
>> having an assert in there would not hurt, since it might help diagnose issues
>> that cross user-kernel boundaries.
>>
>> Thoughts ?
> 
> Hi,
> 
> I understand the intent, but I don't feel these type of kernel tests/checks
> belong in the code.
> 
> However, if there is a specific concern that our use of poll() could highlight
> this type of bug (in the kernel or in the interactions with lttng-modules),
> we can add tests for that.

There is no specific concern. We can remove the assert.

Thanks,

Mathieu

> 
> Thanks,
> Jérémie
> 
>>
>> Mathieu
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com
> 
> 
> --
> Jérémie Galarneau
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
       [not found]         ` <1869646214.7612.1553529371615.JavaMail.zimbra@efficios.com>
@ 2019-03-25 15:59           ` Yannick Lamarre
  0 siblings, 0 replies; 12+ messages in thread
From: Yannick Lamarre @ 2019-03-25 15:59 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: lttng-dev, jgalar

Alright, I'll remove the assert for v4.

----- Original Message -----
From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
To: "Jeremie Galarneau" <jeremie.galarneau@efficios.com>
Cc: "Yannick Lamarre" <ylamarre@efficios.com>, "lttng-dev" <lttng-dev@lists.lttng.org>, "jgalar" <jgalar@efficios.com>
Sent: Monday, March 25, 2019 11:56:11 AM
Subject: Re: [lttng-dev] [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll

----- On Mar 25, 2019, at 11:10 AM, Jeremie Galarneau jeremie.galarneau@efficios.com wrote:

> On Fri, 22 Mar 2019 at 11:04, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> ----- On Mar 21, 2019, at 4:25 PM, Jeremie Galarneau
>> jeremie.galarneau@efficios.com wrote:
>>
>> [...]
>> >> +               assert(idx < events->wait.nb_fd);
>> >
>> > Not needed. Using assert() is fine to highlight assumptions across
>> > interfaces/functions and/or catch catastrophic internal errors. Here,
>> > this would be a very local condition that seems to be already
>> > enforced.
>> >
>> > Reworking the 'for' condition to use 'i < events->wait.nb_fd' and
>> > breaking out early of the loop is also an option here.
>>
>> The reason for using assert() here is to ensure that there is no discrepancy
>> between the number of active poll fd in the set and the value returned by
>> poll indicating that same number of active fds.
>>
>> We'd need a kernel bug (or memory corruption) to have a mismatch, but I thought
>> having an assert in there would not hurt, since it might help diagnose issues
>> that cross user-kernel boundaries.
>>
>> Thoughts ?
> 
> Hi,
> 
> I understand the intent, but I don't feel these type of kernel tests/checks
> belong in the code.
> 
> However, if there is a specific concern that our use of poll() could highlight
> this type of bug (in the kernel or in the interactions with lttng-modules),
> we can add tests for that.

There is no specific concern. We can remove the assert.

Thanks,

Mathieu

> 
> Thanks,
> Jérémie
> 
>>
>> Mathieu
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com
> 
> 
> --
> Jérémie Galarneau
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-03-25 15:59 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190321150453.7312-1-ylamarre@efficios.com>
2019-03-21 15:04 ` [PATCH lttng-tools v2] Add Unit test to poll compatibility layer Yannick Lamarre
2019-03-21 15:04 ` [PATCH lttng-tools v2] Change LTTNG_POLL_GETNB behaviour for poll flavor Yannick Lamarre
2019-03-21 15:04 ` [PATCH lttng-tools v2] Adapt poll layer behaviour to match the epoll layer Yannick Lamarre
2019-03-21 15:04 ` [PATCH lttng-tools v2] Fix hang in thread_rotation when using compat-poll Yannick Lamarre
2019-03-21 15:04 ` [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Yannick Lamarre
2019-03-21 15:04 ` [PATCH lttng-tools v2] Clean code base from redundant verification Yannick Lamarre
2019-03-21 15:04 ` [PATCH lttng-tools v2] Fix typo Yannick Lamarre
     [not found] ` <20190321150453.7312-6-ylamarre@efficios.com>
2019-03-21 20:25   ` [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Jérémie Galarneau
     [not found]   ` <CA+jJMxui0_5EF6XLH-9G90-yJEE63zdiBifTeF4i9iQp95D7tg@mail.gmail.com>
2019-03-22 15:04     ` Mathieu Desnoyers
     [not found]     ` <8703543.6154.1553267076349.JavaMail.zimbra@efficios.com>
2019-03-25 15:10       ` Jérémie Galarneau
     [not found]       ` <CA+jJMxsbA0ksOJN9yqbtCVXkTLKOqfEMek6P9_bxSdKH75zEPQ@mail.gmail.com>
2019-03-25 15:56         ` Mathieu Desnoyers
     [not found]         ` <1869646214.7612.1553529371615.JavaMail.zimbra@efficios.com>
2019-03-25 15:59           ` Yannick Lamarre

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.