[PATCH lttng-tools 1/8] Fix: lttng_poll_mod calls compat_(e)poll_add

[PATCH lttng-tools 1/8] Fix: lttng_poll_mod calls compat_(e)poll_add

[Yannick Lamarre]

lttng_poll_mod should call compat_(e)poll_mod.

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

diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index d4bd87f5..fde54ddb 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -177,7 +177,7 @@ extern int compat_epoll_del(struct lttng_poll_event *events, int fd);
 extern int compat_epoll_mod(struct lttng_poll_event *events,
 		int fd, uint32_t req_events);
 #define lttng_poll_mod(events, fd, req_events) \
-	compat_epoll_add(events, fd, req_events)
+	compat_epoll_mod(events, fd, req_events)
 
 /*
  * Set up the poll set limits variable poll_max_size
@@ -361,7 +361,7 @@ extern int compat_poll_del(struct lttng_poll_event *events, int fd);
 extern int compat_poll_mod(struct lttng_poll_event *events,
 		int fd, uint32_t req_events);
 #define lttng_poll_mod(events, fd, req_events) \
-	compat_poll_add(events, fd, req_events)
+	compat_poll_mod(events, fd, req_events)
 
 /*
  * Set up the poll set limits variable poll_max_size
-- 
2.11.0

[PATCH lttng-tools 2/8] Add Unit test to poll compatibility layer

[Yannick Lamarre]

Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
 .gitignore                          |   1 +
 tests/unit/Makefile.am              |   9 +-
 tests/unit/test_utils_compat_poll.c | 246 ++++++++++++++++++++++++++++++++++++
 3 files changed, 255 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..f1d756d3
--- /dev/null
+++ b/tests/unit/test_utils_compat_poll.c
@@ -0,0 +1,246 @@
+/*
+ * test_utils_compat_poll.c
+ *
+ * Unit tests for the compatibility layer of poll/epoll API.
+ *
+ * Copyright (C) 2019 Yannick Lamarre <ylamarre@efficios.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#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 44
+#else
+#define NUM_TESTS 43
+#endif
+
+#ifdef HAVE_EPOLL
+#if defined(HAVE_EPOLL_CREATE1) && defined(EPOLL_CLOEXEC)
+#define CLOE_VALUE EPOLL_CLOEXEC
+#else
+#define CLOE_VALUE FD_CLOEXEC
+#endif
+
+void test_epoll_compat()
+{
+	/*
+	 * 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()
+{
+	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 imcompatibility 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()
+{
+	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()
+{
+	struct lttng_poll_event poll_events;
+	struct lttng_poll_event cpoll_events;
+	/* Code unshamelly taken from pipe(2) man page */
+	int hupfd[2];
+	int infd[2];
+	pid_t cpid;
+	char rbuf, tbuf = 0xCA;
+	int wstatus;
+
+	lttng_poll_init(&poll_events);
+	lttng_poll_init(&cpoll_events);
+	if (pipe(hupfd) == -1) {
+		diag("We should really abort the test...");
+	}
+
+	if (pipe(infd) == -1) {
+		diag("We should really abort the test...");
+	}
+
+	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 == (char)0xCA, "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");
+		tbuf = 0xCA;
+		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()
+{
+#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
+
+	/* Typecast to turn off -Waddress warnings */
+	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(int argc, const char *argv[])
+{
+	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

[PATCH lttng-tools 3/8] Change LTTNG_POLL_GETNB behaviour for poll flavor

[Yannick Lamarre]

LTTNG_POLL_GETNB was modified 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 still providing test compatibility.

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

diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index fde54ddb..7dd8741b 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -315,10 +315,14 @@ 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 was modified 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 still providing test compatibility.
  */
 #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

[PATCH lttng-tools 4/8] Adapt poll layer behaviour to match the epoll layer

[Yannick Lamarre]

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 7dd8741b..a8a08d9e 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

[PATCH lttng-tools 5/8] Fix hang in thread_rotation when using compat-poll

[Yannick Lamarre]

The poll flavor of the lttng_poll api requires the caller to verify
which of the fd has really waken the thread from wait. This
verification was missing and a blocking read was made 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

[PATCH lttng-tools 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll

[Yannick Lamarre]

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 | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
index b7c17df7..27e2002b 100644
--- a/src/common/compat/compat-poll.c
+++ b/src/common/compat/compat-poll.c
@@ -292,6 +292,8 @@ error:
 int compat_poll_wait(struct lttng_poll_event *events, int timeout)
 {
 	int ret;
+	int count = 0, idx;
+	struct pollfd swapfd;
 
 	if (events == NULL || events->current.events == NULL) {
 		ERR("poll wait arguments error");
@@ -324,10 +326,28 @@ 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 cpmpat-epoll behaviour.
 	 */
-	return events->wait.nb_fd;
+	if ( ret != events->wait.nb_fd && ret != 0) {
+		while (count < ret && events->wait.events[count].revents != 0) {
+			count += 1;
+		}
+		idx = count + 1;
+		while (count < ret) {
+			if (events->wait.events[idx].revents != 0) {
+				swapfd = events->wait.events[idx];
+				events->wait.events[idx] = events->wait.events[count];
+				events->wait.events[count] = swapfd;
+				count += 1;
+			}
+			if (idx < (events->wait.nb_fd - 1)) {
+				idx += 1;
+			}
+		}
+	}
+
+	return ret;
 
 error:
 	return -1;
-- 
2.11.0

[PATCH lttng-tools 7/8] Clean code base from redundant verification

[Yannick Lamarre]

Removed redundant verification for file descriptors with 0 revents in
code code base now that lttng_poll_wait presents the FDs à la epoll.

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

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

[PATCH lttng-tools 8/8] Fix typo

[Yannick Lamarre]

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 a8a08d9e..4272374c 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -361,7 +361,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

Re: [PATCH lttng-tools 2/8] Add Unit test to poll compatibility layer

[Mathieu Desnoyers]

----- On Mar 19, 2019, at 5:17 PM, Yannick Lamarre ylamarre@efficios.com wrote:

> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> .gitignore                          |   1 +
> tests/unit/Makefile.am              |   9 +-
> tests/unit/test_utils_compat_poll.c | 246 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 255 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..f1d756d3
> --- /dev/null
> +++ b/tests/unit/test_utils_compat_poll.c
> @@ -0,0 +1,246 @@
> +/*
> + * test_utils_compat_poll.c
> + *
> + * Unit tests for the compatibility layer of poll/epoll API.
> + *
> + * Copyright (C) 2019 Yannick Lamarre <ylamarre@efficios.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> THE
> + * SOFTWARE.

Tests in lttng-tools are GPLv2.

> + */
> +
> +#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 44
> +#else
> +#define NUM_TESTS 43
> +#endif
> +
> +#ifdef HAVE_EPOLL
> +#if defined(HAVE_EPOLL_CREATE1) && defined(EPOLL_CLOEXEC)
> +#define CLOE_VALUE EPOLL_CLOEXEC
> +#else
> +#define CLOE_VALUE FD_CLOEXEC
> +#endif
> +
> +void test_epoll_compat()

In C, you want (void) here. () is same as (...)   (unlike c++)

Applies everywhere when functions don't take parameters.

> +{
> +	/*
> +	 * Type conversion present to disable warning of anonymous enum from
> +	 * compiler

End comments with ".".

> +	 */
> +	ok((int)LTTNG_CLOEXEC == (int)CLOE_VALUE, "epoll's CLOEXEC value");
> +}
> +#endif
> +
> +void test_alloc()
> +{
> +	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 imcompatibility across

incompatibility

> +	 * 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()
> +{
> +	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()
> +{
> +	struct lttng_poll_event poll_events;
> +	struct lttng_poll_event cpoll_events;
> +	/* Code unshamelly taken from pipe(2) man page */

No need for this comment.

> +	int hupfd[2];
> +	int infd[2];
> +	pid_t cpid;
> +	char rbuf, tbuf = 0xCA;

What is the purpose of 0xCA for tbuf ?

Also we try to never hardcode constants in code. At least we #define it
at the top of the file or in a header with a comment.

> +	int wstatus;
> +
> +	lttng_poll_init(&poll_events);
> +	lttng_poll_init(&cpoll_events);
> +	if (pipe(hupfd) == -1) {
> +		diag("We should really abort the test...");

Then you might want to call abort(), or, better, check this is a ok() test ?

> +	}
> +
> +	if (pipe(infd) == -1) {
> +		diag("We should really abort the test...");

Likewise.

> +	}
> +
> +	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 == (char)0xCA, "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");
> +		tbuf = 0xCA;

Purpose of setting tbuf to 0xCA here ?

> +		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()
> +{
> +#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
> +
> +	/* Typecast to turn off -Waddress warnings */

This above looks like a stale comment.

> +	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(int argc, const char *argv[])

considering argc/argv are unused, the standard allows int main(void), which
is preferred in that situation to ensure we don't have "unused" variables from
static checkers.

Thanks,

Mathieu

> +{
> +	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
> 
> _______________________________________________
> 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: [PATCH lttng-tools 3/8] Change LTTNG_POLL_GETNB behaviour for poll flavor

[Mathieu Desnoyers]

----- On Mar 19, 2019, at 5:17 PM, Yannick Lamarre ylamarre@efficios.com wrote:

> LTTNG_POLL_GETNB was modified to provide compatibility with the epoll

LTTNG_POLL_GETNB was modified -> Modify LTTNG_POLL_GETNB...

(use present, not past)

> 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 still providing test compatibility.

still providing test compatibility -> ensuring similar API behavior between
compatibility layer implementations.

> 
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> src/common/compat/poll.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
> index fde54ddb..7dd8741b 100644
> --- a/src/common/compat/poll.h
> +++ b/src/common/compat/poll.h
> @@ -315,10 +315,14 @@ 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 was modified to provide compatibility with the epoll

No reason to describe the evolution of the code in a comment. We document
what is there now.

Thanks,

Mathieu

> + * 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 still providing test compatibility.
>  */
> #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
> 
> _______________________________________________
> 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: [PATCH lttng-tools 5/8] Fix hang in thread_rotation when using compat-poll

[Mathieu Desnoyers]

----- On Mar 19, 2019, at 5:17 PM, Yannick Lamarre ylamarre@efficios.com wrote:

> The poll flavor of the lttng_poll api requires the caller to verify
> which of the fd has really waken the thread from wait. This
> verification was missing and a blocking read was made on an empty fd.

Please use present tense in the changelog.

Other than that,

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu

> 
> 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
> 
> _______________________________________________
> 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: [PATCH lttng-tools 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll

[Mathieu Desnoyers]

----- On Mar 19, 2019, at 5:17 PM, 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 | 26 +++++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
> index b7c17df7..27e2002b 100644
> --- a/src/common/compat/compat-poll.c
> +++ b/src/common/compat/compat-poll.c
> @@ -292,6 +292,8 @@ error:
> int compat_poll_wait(struct lttng_poll_event *events, int timeout)
> {
> 	int ret;
> +	int count = 0, idx;
> +	struct pollfd swapfd;

The declaration scope of swapfd should be narrower (see below).

> 
> 	if (events == NULL || events->current.events == NULL) {
> 		ERR("poll wait arguments error");
> @@ -324,10 +326,28 @@ 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 cpmpat-epoll behaviour.

cpmpat -> compat

> 	 */
> -	return events->wait.nb_fd;
> +	if ( ret != events->wait.nb_fd && ret != 0) {

remove space after if (.

> +		while (count < ret && events->wait.events[count].revents != 0) {
> +			count += 1;
> +		}
> +		idx = count + 1;
> +		while (count < ret) {
> +			if (events->wait.events[idx].revents != 0) {

You should declare swapfd in this scope.

> +				swapfd = events->wait.events[idx];
> +				events->wait.events[idx] = events->wait.events[count];
> +				events->wait.events[count] = swapfd;
> +				count += 1;
> +			}
> +			if (idx < (events->wait.nb_fd - 1)) {
> +				idx += 1;
> +			}
> +		}

I don't think this algorithm does what is written on the box. :-/

Thanks,

Mathieu


> +	}
> +
> +	return ret;
> 
> error:
> 	return -1;
> --
> 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: [PATCH lttng-tools 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll

[Mathieu Desnoyers]

----- On Mar 20, 2019, at 3:42 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Mar 19, 2019, at 5:17 PM, 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 | 26 +++++++++++++++++++++++---
>> 1 file changed, 23 insertions(+), 3 deletions(-)
>> 
>> diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
>> index b7c17df7..27e2002b 100644
>> --- a/src/common/compat/compat-poll.c
>> +++ b/src/common/compat/compat-poll.c
>> @@ -292,6 +292,8 @@ error:
>> int compat_poll_wait(struct lttng_poll_event *events, int timeout)
>> {
>> 	int ret;
>> +	int count = 0, idx;
>> +	struct pollfd swapfd;
> 
> The declaration scope of swapfd should be narrower (see below).
> 
>> 
>> 	if (events == NULL || events->current.events == NULL) {
>> 		ERR("poll wait arguments error");
>> @@ -324,10 +326,28 @@ 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 cpmpat-epoll behaviour.
> 
> cpmpat -> compat
> 
>> 	 */
>> -	return events->wait.nb_fd;
>> +	if ( ret != events->wait.nb_fd && ret != 0) {
> 
> remove space after if (.
> 
>> +		while (count < ret && events->wait.events[count].revents != 0) {
>> +			count += 1;
>> +		}
>> +		idx = count + 1;
>> +		while (count < ret) {
>> +			if (events->wait.events[idx].revents != 0) {
> 
> You should declare swapfd in this scope.
> 
>> +				swapfd = events->wait.events[idx];
>> +				events->wait.events[idx] = events->wait.events[count];
>> +				events->wait.events[count] = swapfd;
>> +				count += 1;
>> +			}
>> +			if (idx < (events->wait.nb_fd - 1)) {
>> +				idx += 1;
>> +			}
>> +		}
> 
> I don't think this algorithm does what is written on the box. :-/

Based on our in-person discussion, this algorithm seems to work, but
would benefit from clearer variable names, and comments. Let's continue
the discussion on a v2.

Thanks,

Mathieu


> 
> Thanks,
> 
> Mathieu
> 
> 
>> +	}
>> +
>> +	return ret;
>> 
>> error:
>> 	return -1;
>> --
>> 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

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