* [PATCH 2/4] unit: End to end FD passing test
2016-05-04 11:03 [PATCH 1/4] dbus: Close unused file descriptors when creating message Andrew Zaborowski
@ 2016-05-04 11:03 ` Andrew Zaborowski
2016-05-04 16:09 ` Denis Kenzior
2016-05-04 11:03 ` [PATCH 3/4] unit: Add UNIX_FDS header fields in FD test messages Andrew Zaborowski
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Zaborowski @ 2016-05-04 11:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 9098 bytes --]
---
Makefile.am | 3 +
unit/test-dbus-message-fds.c | 368 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 371 insertions(+)
create mode 100644 unit/test-dbus-message-fds.c
diff --git a/Makefile.am b/Makefile.am
index 3e8e3ef..aa99bdd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -140,6 +140,7 @@ dbus_tests = unit/test-kdbus \
unit/test-dbus \
unit/test-dbus-util \
unit/test-dbus-message \
+ unit/test-dbus-message-fds \
unit/test-dbus-service \
unit/test-dbus-watch \
unit/test-dbus-properties \
@@ -186,6 +187,8 @@ unit_test_dbus_LDADD = ell/libell-private.la
unit_test_dbus_message_LDADD = ell/libell-private.la
+unit_test_dbus_message_fds_LDADD = ell/libell-private.la
+
unit_test_dbus_util_LDADD = ell/libell-private.la
unit_test_dbus_service_LDADD = ell/libell-private.la
diff --git a/unit/test-dbus-message-fds.c b/unit/test-dbus-message-fds.c
new file mode 100644
index 0000000..033d0a9
--- /dev/null
+++ b/unit/test-dbus-message-fds.c
@@ -0,0 +1,368 @@
+/*
+ *
+ * Embedded Linux library
+ *
+ * Copyright (C) 2011-2014 Intel Corporation. All rights reserved.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <stdio.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include <ell/ell.h>
+#include <ell/dbus-private.h>
+
+#define TEST_BUS_ADDRESS "unix:path=/tmp/ell-test-bus"
+
+static pid_t dbus_daemon_pid = -1;
+
+static int kdbus_fd = -1;
+
+static char bus_address[128];
+
+static void start_dbus_daemon(void)
+{
+ char *prg_argv[6];
+ char *prg_envp[1];
+ pid_t pid;
+
+ prg_argv[0] = "/usr/bin/dbus-daemon";
+ prg_argv[1] = "--session";
+ prg_argv[2] = "--address=" TEST_BUS_ADDRESS;
+ prg_argv[3] = "--nopidfile";
+ prg_argv[4] = "--nofork";
+ prg_argv[5] = NULL;
+
+ prg_envp[0] = NULL;
+
+ l_info("launching dbus-daemon");
+
+ pid = fork();
+ if (pid < 0) {
+ l_error("failed to fork new process");
+ return;
+ }
+
+ if (pid == 0) {
+ execve(prg_argv[0], prg_argv, prg_envp);
+ exit(EXIT_SUCCESS);
+ }
+
+ l_info("dbus-daemon process %d created", pid);
+
+ dbus_daemon_pid = pid;
+
+ strcpy(bus_address, TEST_BUS_ADDRESS);
+}
+
+static void create_kdbus(void)
+{
+ char bus_name[64];
+
+ snprintf(bus_name, sizeof(bus_name), "%u-ell-test", getuid());
+
+ kdbus_fd = _dbus_kernel_create_bus(bus_name);
+ if (kdbus_fd < 0) {
+ l_warn("kdbus not available");
+ return;
+ }
+
+ snprintf(bus_address, sizeof(bus_address),
+ "kernel:path=/dev/kdbus/%s/bus", bus_name);
+}
+
+static void signal_handler(struct l_signal *signal, uint32_t signo,
+ void *user_data)
+{
+ switch (signo) {
+ case SIGINT:
+ case SIGTERM:
+ l_info("Terminate");
+ l_main_quit();
+ break;
+ case SIGCHLD:
+ while (1) {
+ pid_t pid;
+ int status;
+
+ pid = waitpid(WAIT_ANY, &status, WNOHANG);
+ if (pid < 0 || pid == 0)
+ break;
+
+ l_info("process %d terminated with status=%d\n",
+ pid, status);
+
+ if (pid == dbus_daemon_pid) {
+ dbus_daemon_pid = -1;
+ l_main_quit();
+ }
+ }
+ break;
+ }
+}
+
+static struct l_dbus *dbus;
+
+struct dbus_test {
+ const char *name;
+ void (*start)(struct l_dbus *dbus, void *);
+ void *data;
+};
+
+static bool success;
+static struct l_queue *tests;
+
+static void test_add(const char *name,
+ void (*start)(struct l_dbus *dbus, void *),
+ void *test_data)
+{
+ struct dbus_test *test = l_new(struct dbus_test, 1);
+
+ test->name = name;
+ test->start = start;
+ test->data = test_data;
+
+ if (!tests)
+ tests = l_queue_new();
+
+ l_queue_push_tail(tests, test);
+}
+
+static void test_next()
+{
+ struct dbus_test *test = l_queue_pop_head(tests);
+
+ if (!test) {
+ success = true;
+ l_main_quit();
+ return;
+ }
+
+ l_info("TEST: %s", test->name);
+
+ test->start(dbus, test->data);
+
+ l_free(test);
+}
+
+#define test_assert(cond) \
+ do { \
+ if (!(cond)) { \
+ l_info("TEST FAILED in %s at %s:%i: %s", \
+ __func__, __FILE__, __LINE__, \
+ L_STRINGIFY(cond)); \
+ l_main_quit(); \
+ return; \
+ } \
+ } while (0)
+
+static void request_name_callback(struct l_dbus *dbus, bool success,
+ bool queued, void *user_data)
+{
+ l_info("request name result=%s",
+ success ? (queued ? "queued" : "success") : "failed");
+
+ test_next();
+}
+
+static void ready_callback(void *user_data)
+{
+ l_info("ready");
+
+ l_dbus_name_acquire(dbus, "org.test", false, false, false,
+ request_name_callback, NULL);
+}
+
+static void disconnect_callback(void *user_data)
+{
+ l_info("Disconnected from DBus");
+ l_main_quit();
+}
+
+static struct l_dbus_message *get_random_callback(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ void *user_data)
+{
+ struct l_dbus_message *reply;
+ int fd;
+
+ reply = l_dbus_message_new_method_return(message);
+
+ fd = open("/dev/random", O_RDONLY);
+ l_dbus_message_set_arguments(reply, "h", fd);
+ close(fd);
+
+ return reply;
+}
+
+static void setup_test_interface(struct l_dbus_interface *interface)
+{
+ l_dbus_interface_method(interface, "GetRandom", 0, get_random_callback,
+ "h", "", "randomfd");
+}
+
+static int count_fds(void)
+{
+ int fd;
+ int count = 0;
+
+ for (fd = 0; fd < FD_SETSIZE; fd++)
+ if (fcntl(fd, F_GETFL) != -1 || errno != EBADF)
+ count++;
+
+ return count;
+}
+
+static bool compare_failed;
+
+static void compare_files(int a, int b)
+{
+ struct stat sa, sb;
+
+ compare_failed = true;
+
+ test_assert(fstat(a, &sa) == 0);
+ test_assert(fstat(b, &sb) == 0);
+
+ test_assert(sa.st_dev == sb.st_dev);
+ test_assert(sa.st_ino == sb.st_ino);
+ test_assert(sa.st_rdev == sb.st_rdev);
+
+ compare_failed = false;
+}
+
+static int open_fds;
+
+static void get_random_idle_callback(void *user_data)
+{
+ test_assert(count_fds() == open_fds);
+
+ test_next();
+}
+
+static void get_random_return_callback(struct l_dbus_message *message,
+ void *user_data)
+{
+ int fd0, fd1;
+
+ test_assert(!l_dbus_message_get_error(message, NULL, NULL));
+
+ test_assert(l_dbus_message_get_arguments(message, "h", &fd1));
+
+ fd0 = open("/dev/random", O_RDONLY);
+ test_assert(fd0 != -1);
+
+ compare_files(fd0, fd1);
+ if (compare_failed)
+ return;
+
+ close(fd0);
+ close(fd1);
+
+ test_assert(l_idle_oneshot(get_random_idle_callback, NULL, NULL));
+}
+
+static void test_fd_passing_1(struct l_dbus *dbus, void *test_data)
+{
+ open_fds = count_fds();
+
+ l_dbus_method_call(dbus, "org.test", "/test", "org.test", "GetRandom",
+ NULL, get_random_return_callback, NULL, NULL);
+}
+
+int main(int argc, char *argv[])
+{
+ struct l_signal *signal;
+ sigset_t mask;
+ int i;
+ bool kdbus = false;
+
+ for (i = 1; i < argc; i++) {
+ if (!strcmp(argv[i], "--kdbus"))
+ kdbus = true;
+ }
+
+ sigemptyset(&mask);
+ sigaddset(&mask, SIGINT);
+ sigaddset(&mask, SIGTERM);
+ sigaddset(&mask, SIGCHLD);
+
+ signal = l_signal_create(&mask, signal_handler, NULL, NULL);
+
+ l_log_set_stderr();
+
+ if (kdbus)
+ create_kdbus();
+ else
+ start_dbus_daemon();
+
+ if (!bus_address[0])
+ return -1;
+
+ for (i = 0; i < 10; i++) {
+ usleep(200 * 1000);
+
+ dbus = l_dbus_new(bus_address);
+ if (dbus)
+ break;
+ }
+
+ l_dbus_set_ready_handler(dbus, ready_callback, dbus, NULL);
+ l_dbus_set_disconnect_handler(dbus, disconnect_callback, NULL, NULL);
+
+ if (!l_dbus_register_interface(dbus, "org.test", setup_test_interface,
+ NULL, true)) {
+ l_info("Unable to register interface");
+ goto done;
+ }
+
+ if (!l_dbus_object_add_interface(dbus, "/test", "org.test", NULL)) {
+ l_info("Unable to instantiate interface");
+ goto done;
+ }
+
+ test_add("FD passing 1", test_fd_passing_1, NULL);
+
+ l_main_run();
+
+ l_queue_destroy(tests, l_free);
+
+done:
+ l_dbus_destroy(dbus);
+
+ if (dbus_daemon_pid > 0)
+ kill(dbus_daemon_pid, SIGKILL);
+
+ if (kdbus_fd >= 0)
+ close(kdbus_fd);
+
+ l_signal_remove(signal);
+
+ if (!success)
+ abort();
+
+ return 0;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/4] unit: End to end FD passing test
2016-05-04 11:03 ` [PATCH 2/4] unit: End to end FD passing test Andrew Zaborowski
@ 2016-05-04 16:09 ` Denis Kenzior
0 siblings, 0 replies; 9+ messages in thread
From: Denis Kenzior @ 2016-05-04 16:09 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 10248 bytes --]
Hi Andrew,
On 05/04/2016 06:03 AM, Andrew Zaborowski wrote:
> ---
> Makefile.am | 3 +
> unit/test-dbus-message-fds.c | 368 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 371 insertions(+)
> create mode 100644 unit/test-dbus-message-fds.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 3e8e3ef..aa99bdd 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -140,6 +140,7 @@ dbus_tests = unit/test-kdbus \
> unit/test-dbus \
> unit/test-dbus-util \
> unit/test-dbus-message \
> + unit/test-dbus-message-fds \
> unit/test-dbus-service \
> unit/test-dbus-watch \
> unit/test-dbus-properties \
> @@ -186,6 +187,8 @@ unit_test_dbus_LDADD = ell/libell-private.la
>
> unit_test_dbus_message_LDADD = ell/libell-private.la
>
> +unit_test_dbus_message_fds_LDADD = ell/libell-private.la
> +
> unit_test_dbus_util_LDADD = ell/libell-private.la
>
> unit_test_dbus_service_LDADD = ell/libell-private.la
> diff --git a/unit/test-dbus-message-fds.c b/unit/test-dbus-message-fds.c
> new file mode 100644
> index 0000000..033d0a9
> --- /dev/null
> +++ b/unit/test-dbus-message-fds.c
> @@ -0,0 +1,368 @@
> +/*
> + *
> + * Embedded Linux library
> + *
> + * Copyright (C) 2011-2014 Intel Corporation. All rights reserved.
Please remember to update the copyright when copy-pasting stuff.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <sys/wait.h>
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include <ell/ell.h>
> +#include <ell/dbus-private.h>
> +
> +#define TEST_BUS_ADDRESS "unix:path=/tmp/ell-test-bus"
> +
> +static pid_t dbus_daemon_pid = -1;
> +
> +static int kdbus_fd = -1;
> +
> +static char bus_address[128];
> +
> +static void start_dbus_daemon(void)
> +{
> + char *prg_argv[6];
> + char *prg_envp[1];
> + pid_t pid;
> +
> + prg_argv[0] = "/usr/bin/dbus-daemon";
> + prg_argv[1] = "--session";
> + prg_argv[2] = "--address=" TEST_BUS_ADDRESS;
> + prg_argv[3] = "--nopidfile";
> + prg_argv[4] = "--nofork";
> + prg_argv[5] = NULL;
> +
> + prg_envp[0] = NULL;
> +
> + l_info("launching dbus-daemon");
> +
> + pid = fork();
> + if (pid < 0) {
> + l_error("failed to fork new process");
> + return;
> + }
> +
> + if (pid == 0) {
> + execve(prg_argv[0], prg_argv, prg_envp);
> + exit(EXIT_SUCCESS);
> + }
> +
> + l_info("dbus-daemon process %d created", pid);
> +
> + dbus_daemon_pid = pid;
> +
> + strcpy(bus_address, TEST_BUS_ADDRESS);
> +}
> +
> +static void create_kdbus(void)
> +{
> + char bus_name[64];
> +
> + snprintf(bus_name, sizeof(bus_name), "%u-ell-test", getuid());
> +
> + kdbus_fd = _dbus_kernel_create_bus(bus_name);
> + if (kdbus_fd < 0) {
> + l_warn("kdbus not available");
> + return;
> + }
> +
> + snprintf(bus_address, sizeof(bus_address),
> + "kernel:path=/dev/kdbus/%s/bus", bus_name);
> +}
> +
> +static void signal_handler(struct l_signal *signal, uint32_t signo,
> + void *user_data)
> +{
> + switch (signo) {
> + case SIGINT:
> + case SIGTERM:
> + l_info("Terminate");
> + l_main_quit();
> + break;
> + case SIGCHLD:
> + while (1) {
> + pid_t pid;
> + int status;
> +
> + pid = waitpid(WAIT_ANY, &status, WNOHANG);
> + if (pid < 0 || pid == 0)
> + break;
> +
> + l_info("process %d terminated with status=%d\n",
> + pid, status);
> +
> + if (pid == dbus_daemon_pid) {
> + dbus_daemon_pid = -1;
> + l_main_quit();
> + }
> + }
> + break;
> + }
> +}
> +
> +static struct l_dbus *dbus;
> +
> +struct dbus_test {
> + const char *name;
> + void (*start)(struct l_dbus *dbus, void *);
> + void *data;
> +};
> +
> +static bool success;
> +static struct l_queue *tests;
> +
> +static void test_add(const char *name,
> + void (*start)(struct l_dbus *dbus, void *),
> + void *test_data)
> +{
> + struct dbus_test *test = l_new(struct dbus_test, 1);
> +
> + test->name = name;
> + test->start = start;
> + test->data = test_data;
> +
> + if (!tests)
> + tests = l_queue_new();
> +
> + l_queue_push_tail(tests, test);
> +}
> +
> +static void test_next()
> +{
> + struct dbus_test *test = l_queue_pop_head(tests);
> +
> + if (!test) {
> + success = true;
> + l_main_quit();
> + return;
> + }
> +
> + l_info("TEST: %s", test->name);
> +
> + test->start(dbus, test->data);
> +
> + l_free(test);
> +}
> +
> +#define test_assert(cond) \
> + do { \
> + if (!(cond)) { \
> + l_info("TEST FAILED in %s at %s:%i: %s", \
> + __func__, __FILE__, __LINE__, \
> + L_STRINGIFY(cond)); \
> + l_main_quit(); \
> + return; \
> + } \
> + } while (0)
> +
> +static void request_name_callback(struct l_dbus *dbus, bool success,
> + bool queued, void *user_data)
> +{
> + l_info("request name result=%s",
> + success ? (queued ? "queued" : "success") : "failed");
> +
> + test_next();
> +}
> +
> +static void ready_callback(void *user_data)
> +{
> + l_info("ready");
> +
> + l_dbus_name_acquire(dbus, "org.test", false, false, false,
> + request_name_callback, NULL);
> +}
> +
> +static void disconnect_callback(void *user_data)
> +{
> + l_info("Disconnected from DBus");
> + l_main_quit();
> +}
> +
> +static struct l_dbus_message *get_random_callback(struct l_dbus *dbus,
> + struct l_dbus_message *message,
> + void *user_data)
> +{
> + struct l_dbus_message *reply;
> + int fd;
> +
> + reply = l_dbus_message_new_method_return(message);
> +
> + fd = open("/dev/random", O_RDONLY);
> + l_dbus_message_set_arguments(reply, "h", fd);
> + close(fd);
> +
> + return reply;
> +}
> +
> +static void setup_test_interface(struct l_dbus_interface *interface)
> +{
> + l_dbus_interface_method(interface, "GetRandom", 0, get_random_callback,
> + "h", "", "randomfd");
> +}
> +
> +static int count_fds(void)
> +{
> + int fd;
> + int count = 0;
> +
> + for (fd = 0; fd < FD_SETSIZE; fd++)
> + if (fcntl(fd, F_GETFL) != -1 || errno != EBADF)
> + count++;
> +
> + return count;
> +}
> +
> +static bool compare_failed;
> +
> +static void compare_files(int a, int b)
> +{
> + struct stat sa, sb;
> +
> + compare_failed = true;
> +
> + test_assert(fstat(a, &sa) == 0);
> + test_assert(fstat(b, &sb) == 0);
> +
> + test_assert(sa.st_dev == sb.st_dev);
> + test_assert(sa.st_ino == sb.st_ino);
> + test_assert(sa.st_rdev == sb.st_rdev);
> +
> + compare_failed = false;
> +}
> +
> +static int open_fds;
> +
> +static void get_random_idle_callback(void *user_data)
> +{
> + test_assert(count_fds() == open_fds);
> +
> + test_next();
> +}
> +
> +static void get_random_return_callback(struct l_dbus_message *message,
> + void *user_data)
> +{
> + int fd0, fd1;
> +
> + test_assert(!l_dbus_message_get_error(message, NULL, NULL));
> +
> + test_assert(l_dbus_message_get_arguments(message, "h", &fd1));
> +
> + fd0 = open("/dev/random", O_RDONLY);
> + test_assert(fd0 != -1);
> +
> + compare_files(fd0, fd1);
> + if (compare_failed)
> + return;
> +
> + close(fd0);
> + close(fd1);
> +
> + test_assert(l_idle_oneshot(get_random_idle_callback, NULL, NULL));
> +}
> +
> +static void test_fd_passing_1(struct l_dbus *dbus, void *test_data)
> +{
> + open_fds = count_fds();
> +
> + l_dbus_method_call(dbus, "org.test", "/test", "org.test", "GetRandom",
> + NULL, get_random_return_callback, NULL, NULL);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + struct l_signal *signal;
> + sigset_t mask;
> + int i;
> + bool kdbus = false;
> +
> + for (i = 1; i < argc; i++) {
> + if (!strcmp(argv[i], "--kdbus"))
> + kdbus = true;
unit tests should not take arguments. Can we simply always run the test
with dbus-1 and kdbus? If kdbus is not available, then the test can be
skipped. Similar to how test-kdbus does it.
> + }
> +
> + sigemptyset(&mask);
> + sigaddset(&mask, SIGINT);
> + sigaddset(&mask, SIGTERM);
> + sigaddset(&mask, SIGCHLD);
> +
> + signal = l_signal_create(&mask, signal_handler, NULL, NULL);
> +
> + l_log_set_stderr();
> +
> + if (kdbus)
> + create_kdbus();
> + else
> + start_dbus_daemon();
> +
> + if (!bus_address[0])
> + return -1;
> +
> + for (i = 0; i < 10; i++) {
> + usleep(200 * 1000);
> +
> + dbus = l_dbus_new(bus_address);
> + if (dbus)
> + break;
> + }
> +
> + l_dbus_set_ready_handler(dbus, ready_callback, dbus, NULL);
> + l_dbus_set_disconnect_handler(dbus, disconnect_callback, NULL, NULL);
> +
> + if (!l_dbus_register_interface(dbus, "org.test", setup_test_interface,
> + NULL, true)) {
> + l_info("Unable to register interface");
> + goto done;
> + }
> +
> + if (!l_dbus_object_add_interface(dbus, "/test", "org.test", NULL)) {
> + l_info("Unable to instantiate interface");
> + goto done;
> + }
> +
> + test_add("FD passing 1", test_fd_passing_1, NULL);
> +
> + l_main_run();
> +
> + l_queue_destroy(tests, l_free);
> +
> +done:
> + l_dbus_destroy(dbus);
> +
> + if (dbus_daemon_pid > 0)
> + kill(dbus_daemon_pid, SIGKILL);
> +
> + if (kdbus_fd >= 0)
> + close(kdbus_fd);
> +
> + l_signal_remove(signal);
> +
> + if (!success)
> + abort();
> +
> + return 0;
> +}
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] unit: Add UNIX_FDS header fields in FD test messages
2016-05-04 11:03 [PATCH 1/4] dbus: Close unused file descriptors when creating message Andrew Zaborowski
2016-05-04 11:03 ` [PATCH 2/4] unit: End to end FD passing test Andrew Zaborowski
@ 2016-05-04 11:03 ` Andrew Zaborowski
2016-05-04 16:10 ` Denis Kenzior
2016-05-04 11:03 ` [PATCH 4/4] dbus: Handle partial reads and writes Andrew Zaborowski
2016-05-04 16:05 ` [PATCH 1/4] dbus: Close unused file descriptors when creating message Denis Kenzior
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Zaborowski @ 2016-05-04 11:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]
---
unit/test-dbus-message.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/unit/test-dbus-message.c b/unit/test-dbus-message.c
index 52df9d8..0c14a7b 100644
--- a/unit/test-dbus-message.c
+++ b/unit/test-dbus-message.c
@@ -2608,21 +2608,22 @@ static void compare_files(int a, int b)
static const unsigned char message_binary_basic_h[] = {
0x6c, 0x01, 0x00, 0x01, 0x04, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x6f, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x78, 0x00, 0x00, 0x00,
0x01, 0x01, 0x6f, 0x00, 0x13, 0x00, 0x00, 0x00,
0x2f, 0x63, 0x6f, 0x6d, 0x2f, 0x65, 0x78, 0x61,
0x6d, 0x70, 0x6c, 0x65, 0x2f, 0x6f, 0x62, 0x6a,
0x65, 0x63, 0x74, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x06, 0x01, 0x73, 0x00, 0x0b, 0x00, 0x00, 0x00,
- 0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
- 0x70, 0x6c, 0x65, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x03, 0x01, 0x73, 0x00, 0x06, 0x00, 0x00, 0x00,
+ 0x6d, 0x65, 0x74, 0x68, 0x6f, 0x64, 0x00, 0x00,
0x02, 0x01, 0x73, 0x00, 0x15, 0x00, 0x00, 0x00,
0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
0x70, 0x6c, 0x65, 0x2e, 0x69, 0x6e, 0x74, 0x65,
0x72, 0x66, 0x61, 0x63, 0x65, 0x00, 0x00, 0x00,
- 0x03, 0x01, 0x73, 0x00, 0x06, 0x00, 0x00, 0x00,
- 0x6d, 0x65, 0x74, 0x68, 0x6f, 0x64, 0x00, 0x00,
+ 0x06, 0x01, 0x73, 0x00, 0x0b, 0x00, 0x00, 0x00,
+ 0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
+ 0x70, 0x6c, 0x65, 0x00, 0x00, 0x00, 0x00, 0x00,
0x08, 0x01, 0x67, 0x00, 0x01, 0x68, 0x00, 0x00,
+ 0x09, 0x01, 0x75, 0x00, 0x01, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00,
};
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/4] dbus: Handle partial reads and writes
2016-05-04 11:03 [PATCH 1/4] dbus: Close unused file descriptors when creating message Andrew Zaborowski
2016-05-04 11:03 ` [PATCH 2/4] unit: End to end FD passing test Andrew Zaborowski
2016-05-04 11:03 ` [PATCH 3/4] unit: Add UNIX_FDS header fields in FD test messages Andrew Zaborowski
@ 2016-05-04 11:03 ` Andrew Zaborowski
2016-05-04 11:10 ` Andrzej Zaborowski
2016-05-04 16:38 ` Denis Kenzior
2016-05-04 16:05 ` [PATCH 1/4] dbus: Close unused file descriptors when creating message Denis Kenzior
3 siblings, 2 replies; 9+ messages in thread
From: Andrew Zaborowski @ 2016-05-04 11:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 6582 bytes --]
Loop until we have received or sent all of the data requested. We pass
MSG_WAITALL to potentially reduce the number of iterations. Make sure
we free body and header in classic_recv_message when
dbus_message_build() returns an error.
Calling sendms and recvmsg multiple times makes it even more tricky to
understand what will happen with the FDs passed as oob data when a
partial read/write happens or when there are multiple messsages with FDs
attached in the queue. What this patch does should be equivalent to
what systemd dbus code does. It seems to assume that the FDs are
attached to the last byte of the message so on a partial write, the FDs
are not written and the full array can be passed to sendmsg() again.
I'm not sure if this is documented anywhere but I've seen an explicit
statement of this at ...
---
ell/dbus.c | 179 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 110 insertions(+), 69 deletions(-)
diff --git a/ell/dbus.c b/ell/dbus.c
index 35378b2..3ca7b87 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -579,63 +579,126 @@ static void classic_free(struct l_dbus *dbus)
l_free(classic);
}
+static bool msg_op(int fd, bool send, struct iovec iov[],
+ int **fds, uint32_t *num_fds)
+{
+ int r;
+ struct msghdr msg;
+ struct cmsghdr *cmsg;
+ uint8_t *fd_buf = NULL;
+ int buf_fds;
+ int iovlen = 2;
+
+ if (send)
+ buf_fds = *num_fds;
+ else {
+ buf_fds = 16;
+ *num_fds = 0;
+ }
+
+ if (buf_fds) {
+ fd_buf = alloca(CMSG_SPACE(buf_fds * sizeof(int)));
+
+ if (send) {
+ msg.msg_control = fd_buf;
+ msg.msg_controllen = CMSG_LEN(buf_fds * sizeof(int));
+
+ /* Set up fd_buf contents */
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_len = msg.msg_controllen;
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ memcpy(CMSG_DATA(cmsg), *fds, buf_fds * sizeof(int));
+ }
+ }
+
+ while (iovlen) {
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = iov;
+ msg.msg_iovlen = iovlen;
+ msg.msg_control = fd_buf;
+ msg.msg_controllen = fd_buf ?
+ CMSG_LEN(buf_fds * sizeof(int)) : 0;
+
+ if (send)
+ r = sendmsg(fd, &msg, 0);
+ else
+ r = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC | MSG_WAITALL);
+
+ if (r <= 0) {
+ if (r != -1 || errno != EINTR)
+ return false;
+
+ continue;
+ }
+
+ while (iovlen && r >= (int) iov[0].iov_len) {
+ r -= iov[0].iov_len;
+ iov++;
+ iovlen--;
+ }
+
+ if (iovlen) {
+ iov[0].iov_base += r;
+ iov[0].iov_len -= r;
+ }
+
+ if (send)
+ continue;
+
+ for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
+ cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+ int cnt;
+
+ if (cmsg->cmsg_level != SOL_SOCKET ||
+ cmsg->cmsg_type != SCM_RIGHTS)
+ continue;
+
+ cnt = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
+
+ *fds = l_realloc(*fds, (*num_fds + cnt) * sizeof(int));
+
+ memcpy(*fds + *num_fds, CMSG_DATA(cmsg),
+ cnt * sizeof(int));
+
+ *num_fds += cnt;
+ }
+ }
+
+ return true;
+}
+
static bool classic_send_message(struct l_dbus *dbus,
struct l_dbus_message *message)
{
int fd = l_io_get_fd(dbus->io);
- struct msghdr msg;
struct iovec iov[2];
- ssize_t len;
int *fds;
uint32_t num_fds = 0;
- struct cmsghdr *cmsg;
iov[0].iov_base = _dbus_message_get_header(message, &iov[0].iov_len);
iov[1].iov_base = _dbus_message_get_body(message, &iov[1].iov_len);
- memset(&msg, 0, sizeof(msg));
- msg.msg_iov = iov;
- msg.msg_iovlen = 2;
-
if (dbus->support_unix_fd)
fds = _dbus_message_get_fds(message, &num_fds);
- if (num_fds) {
- msg.msg_control = alloca(CMSG_SPACE(num_fds * sizeof(int)));
- msg.msg_controllen = CMSG_LEN(num_fds * sizeof(int));
-
- cmsg = CMSG_FIRSTHDR(&msg);
- cmsg->cmsg_len = msg.msg_controllen;
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- memcpy(CMSG_DATA(cmsg), fds, num_fds * sizeof(int));
- }
-
- len = sendmsg(fd, &msg, 0);
- if (len < 0)
- return false;
-
- return true;
+ return msg_op(fd, true, iov, &fds, &num_fds);
}
static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
{
int fd = l_io_get_fd(dbus->io);
struct dbus_header hdr;
- struct msghdr msg;
struct iovec iov[2];
- struct cmsghdr *cmsg;
ssize_t len;
void *header, *body;
size_t header_size, body_size;
- union {
- uint8_t bytes[CMSG_SPACE(16 * sizeof(int))];
- struct cmsghdr align;
- } fd_buf;
int *fds = NULL;
uint32_t num_fds = 0;
+ struct l_dbus_message *msg;
+ unsigned int i;
- len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
+ len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK | MSG_WAITALL);
if (len != DBUS_HEADER_SIZE)
return NULL;
@@ -650,59 +713,37 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
iov[1].iov_base = body;
iov[1].iov_len = body_size;
- memset(&msg, 0, sizeof(msg));
- msg.msg_iov = iov;
- msg.msg_iovlen = 2;
- msg.msg_control = &fd_buf;
- msg.msg_controllen = sizeof(fd_buf);
-
- len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
- if (len < 0)
- goto cmsg_fail;
-
- for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
- cmsg = CMSG_NXTHDR(&msg, cmsg)) {
- unsigned int i;
-
- if (cmsg->cmsg_level != SOL_SOCKET ||
- cmsg->cmsg_type != SCM_RIGHTS)
- continue;
-
- num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
- fds = (void *) CMSG_DATA(cmsg);
-
- /* Set FD_CLOEXEC on all file descriptors */
- for (i = 0; i < num_fds; i++) {
- long flags;
-
- flags = fcntl(fds[i], F_GETFD, NULL);
- if (flags < 0)
- continue;
-
- if (!(flags & FD_CLOEXEC))
- fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
- }
- }
+ if (!msg_op(fd, false, iov, &fds, &num_fds))
+ goto fail;
if (hdr.endian != DBUS_NATIVE_ENDIAN) {
l_util_debug(dbus->debug_handler,
dbus->debug_data, "Endianness incorrect");
- goto bad_msg;
+ goto fail;
}
if (hdr.version != 1) {
l_util_debug(dbus->debug_handler,
dbus->debug_data, "Protocol version incorrect");
- goto bad_msg;
+ goto fail;
}
- return dbus_message_build(header, header_size, body, body_size,
+ msg = dbus_message_build(header, header_size, body, body_size,
fds, num_fds);
+ if (!msg)
+ goto fail;
-bad_msg:
-cmsg_fail:
- l_free(header);
+ l_free(fds);
+
+ return msg;
+
+fail:
+ for (i = 0; i < num_fds; i++)
+ close(fds[i]);
+
+ l_free(fds);
l_free(body);
+ l_free(header);
return NULL;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 4/4] dbus: Handle partial reads and writes
2016-05-04 11:03 ` [PATCH 4/4] dbus: Handle partial reads and writes Andrew Zaborowski
@ 2016-05-04 11:10 ` Andrzej Zaborowski
2016-05-04 16:38 ` Denis Kenzior
1 sibling, 0 replies; 9+ messages in thread
From: Andrzej Zaborowski @ 2016-05-04 11:10 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 9608 bytes --]
Sorry for the unterminated commit message. For the record here's a
short discussion of how the FDs are received in recvmsg calls,
https://bugs.freedesktop.org/show_bug.cgi?id=80559 . Re-reading it, I
understand we're not doing enough to handle the corner cases but with
this patch we at least do what systemd does.
Best regards
On 4 May 2016 at 13:03, Andrew Zaborowski <andrew.zaborowski@intel.com> wrote:
> Loop until we have received or sent all of the data requested. We pass
> MSG_WAITALL to potentially reduce the number of iterations. Make sure
> we free body and header in classic_recv_message when
> dbus_message_build() returns an error.
>
> Calling sendms and recvmsg multiple times makes it even more tricky to
> understand what will happen with the FDs passed as oob data when a
> partial read/write happens or when there are multiple messsages with FDs
> attached in the queue. What this patch does should be equivalent to
> what systemd dbus code does. It seems to assume that the FDs are
> attached to the last byte of the message so on a partial write, the FDs
> are not written and the full array can be passed to sendmsg() again.
> I'm not sure if this is documented anywhere but I've seen an explicit
> statement of this at ...
> ---
> ell/dbus.c | 179 +++++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 110 insertions(+), 69 deletions(-)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index 35378b2..3ca7b87 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -579,63 +579,126 @@ static void classic_free(struct l_dbus *dbus)
> l_free(classic);
> }
>
> +static bool msg_op(int fd, bool send, struct iovec iov[],
> + int **fds, uint32_t *num_fds)
> +{
> + int r;
> + struct msghdr msg;
> + struct cmsghdr *cmsg;
> + uint8_t *fd_buf = NULL;
> + int buf_fds;
> + int iovlen = 2;
> +
> + if (send)
> + buf_fds = *num_fds;
> + else {
> + buf_fds = 16;
> + *num_fds = 0;
> + }
> +
> + if (buf_fds) {
> + fd_buf = alloca(CMSG_SPACE(buf_fds * sizeof(int)));
> +
> + if (send) {
> + msg.msg_control = fd_buf;
> + msg.msg_controllen = CMSG_LEN(buf_fds * sizeof(int));
> +
> + /* Set up fd_buf contents */
> + cmsg = CMSG_FIRSTHDR(&msg);
> + cmsg->cmsg_len = msg.msg_controllen;
> + cmsg->cmsg_level = SOL_SOCKET;
> + cmsg->cmsg_type = SCM_RIGHTS;
> + memcpy(CMSG_DATA(cmsg), *fds, buf_fds * sizeof(int));
> + }
> + }
> +
> + while (iovlen) {
> + memset(&msg, 0, sizeof(msg));
> + msg.msg_iov = iov;
> + msg.msg_iovlen = iovlen;
> + msg.msg_control = fd_buf;
> + msg.msg_controllen = fd_buf ?
> + CMSG_LEN(buf_fds * sizeof(int)) : 0;
> +
> + if (send)
> + r = sendmsg(fd, &msg, 0);
> + else
> + r = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC | MSG_WAITALL);
> +
> + if (r <= 0) {
> + if (r != -1 || errno != EINTR)
> + return false;
> +
> + continue;
> + }
> +
> + while (iovlen && r >= (int) iov[0].iov_len) {
> + r -= iov[0].iov_len;
> + iov++;
> + iovlen--;
> + }
> +
> + if (iovlen) {
> + iov[0].iov_base += r;
> + iov[0].iov_len -= r;
> + }
> +
> + if (send)
> + continue;
> +
> + for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
> + cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> + int cnt;
> +
> + if (cmsg->cmsg_level != SOL_SOCKET ||
> + cmsg->cmsg_type != SCM_RIGHTS)
> + continue;
> +
> + cnt = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
> +
> + *fds = l_realloc(*fds, (*num_fds + cnt) * sizeof(int));
> +
> + memcpy(*fds + *num_fds, CMSG_DATA(cmsg),
> + cnt * sizeof(int));
> +
> + *num_fds += cnt;
> + }
> + }
> +
> + return true;
> +}
> +
> static bool classic_send_message(struct l_dbus *dbus,
> struct l_dbus_message *message)
> {
> int fd = l_io_get_fd(dbus->io);
> - struct msghdr msg;
> struct iovec iov[2];
> - ssize_t len;
> int *fds;
> uint32_t num_fds = 0;
> - struct cmsghdr *cmsg;
>
> iov[0].iov_base = _dbus_message_get_header(message, &iov[0].iov_len);
> iov[1].iov_base = _dbus_message_get_body(message, &iov[1].iov_len);
>
> - memset(&msg, 0, sizeof(msg));
> - msg.msg_iov = iov;
> - msg.msg_iovlen = 2;
> -
> if (dbus->support_unix_fd)
> fds = _dbus_message_get_fds(message, &num_fds);
>
> - if (num_fds) {
> - msg.msg_control = alloca(CMSG_SPACE(num_fds * sizeof(int)));
> - msg.msg_controllen = CMSG_LEN(num_fds * sizeof(int));
> -
> - cmsg = CMSG_FIRSTHDR(&msg);
> - cmsg->cmsg_len = msg.msg_controllen;
> - cmsg->cmsg_level = SOL_SOCKET;
> - cmsg->cmsg_type = SCM_RIGHTS;
> - memcpy(CMSG_DATA(cmsg), fds, num_fds * sizeof(int));
> - }
> -
> - len = sendmsg(fd, &msg, 0);
> - if (len < 0)
> - return false;
> -
> - return true;
> + return msg_op(fd, true, iov, &fds, &num_fds);
> }
>
> static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
> {
> int fd = l_io_get_fd(dbus->io);
> struct dbus_header hdr;
> - struct msghdr msg;
> struct iovec iov[2];
> - struct cmsghdr *cmsg;
> ssize_t len;
> void *header, *body;
> size_t header_size, body_size;
> - union {
> - uint8_t bytes[CMSG_SPACE(16 * sizeof(int))];
> - struct cmsghdr align;
> - } fd_buf;
> int *fds = NULL;
> uint32_t num_fds = 0;
> + struct l_dbus_message *msg;
> + unsigned int i;
>
> - len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
> + len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK | MSG_WAITALL);
> if (len != DBUS_HEADER_SIZE)
> return NULL;
>
> @@ -650,59 +713,37 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
> iov[1].iov_base = body;
> iov[1].iov_len = body_size;
>
> - memset(&msg, 0, sizeof(msg));
> - msg.msg_iov = iov;
> - msg.msg_iovlen = 2;
> - msg.msg_control = &fd_buf;
> - msg.msg_controllen = sizeof(fd_buf);
> -
> - len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
> - if (len < 0)
> - goto cmsg_fail;
> -
> - for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
> - cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> - unsigned int i;
> -
> - if (cmsg->cmsg_level != SOL_SOCKET ||
> - cmsg->cmsg_type != SCM_RIGHTS)
> - continue;
> -
> - num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
> - fds = (void *) CMSG_DATA(cmsg);
> -
> - /* Set FD_CLOEXEC on all file descriptors */
> - for (i = 0; i < num_fds; i++) {
> - long flags;
> -
> - flags = fcntl(fds[i], F_GETFD, NULL);
> - if (flags < 0)
> - continue;
> -
> - if (!(flags & FD_CLOEXEC))
> - fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
> - }
> - }
> + if (!msg_op(fd, false, iov, &fds, &num_fds))
> + goto fail;
>
> if (hdr.endian != DBUS_NATIVE_ENDIAN) {
> l_util_debug(dbus->debug_handler,
> dbus->debug_data, "Endianness incorrect");
> - goto bad_msg;
> + goto fail;
> }
>
> if (hdr.version != 1) {
> l_util_debug(dbus->debug_handler,
> dbus->debug_data, "Protocol version incorrect");
> - goto bad_msg;
> + goto fail;
> }
>
> - return dbus_message_build(header, header_size, body, body_size,
> + msg = dbus_message_build(header, header_size, body, body_size,
> fds, num_fds);
> + if (!msg)
> + goto fail;
>
> -bad_msg:
> -cmsg_fail:
> - l_free(header);
> + l_free(fds);
> +
> + return msg;
> +
> +fail:
> + for (i = 0; i < num_fds; i++)
> + close(fds[i]);
> +
> + l_free(fds);
> l_free(body);
> + l_free(header);
>
> return NULL;
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 4/4] dbus: Handle partial reads and writes
2016-05-04 11:03 ` [PATCH 4/4] dbus: Handle partial reads and writes Andrew Zaborowski
2016-05-04 11:10 ` Andrzej Zaborowski
@ 2016-05-04 16:38 ` Denis Kenzior
1 sibling, 0 replies; 9+ messages in thread
From: Denis Kenzior @ 2016-05-04 16:38 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 7627 bytes --]
Hi Andrew,
On 05/04/2016 06:03 AM, Andrew Zaborowski wrote:
> Loop until we have received or sent all of the data requested. We pass
> MSG_WAITALL to potentially reduce the number of iterations. Make sure
> we free body and header in classic_recv_message when
> dbus_message_build() returns an error.
>
> Calling sendms and recvmsg multiple times makes it even more tricky to
> understand what will happen with the FDs passed as oob data when a
> partial read/write happens or when there are multiple messsages with FDs
> attached in the queue. What this patch does should be equivalent to
> what systemd dbus code does. It seems to assume that the FDs are
> attached to the last byte of the message so on a partial write, the FDs
> are not written and the full array can be passed to sendmsg() again.
> I'm not sure if this is documented anywhere but I've seen an explicit
> statement of this at ...
I think this is a bad assumption to make. Browsing af_unix.c makes me
think this is not the case.
> ---
> ell/dbus.c | 179 +++++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 110 insertions(+), 69 deletions(-)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index 35378b2..3ca7b87 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -579,63 +579,126 @@ static void classic_free(struct l_dbus *dbus)
> l_free(classic);
> }
>
> +static bool msg_op(int fd, bool send, struct iovec iov[],
> + int **fds, uint32_t *num_fds)
> +{
Please don't do this, the code is so much easier to understand when the
rx and tx paths are separate.
> + int r;
> + struct msghdr msg;
> + struct cmsghdr *cmsg;
> + uint8_t *fd_buf = NULL;
> + int buf_fds;
> + int iovlen = 2;
> +
> + if (send)
> + buf_fds = *num_fds;
> + else {
> + buf_fds = 16;
> + *num_fds = 0;
> + }
> +
> + if (buf_fds) {
> + fd_buf = alloca(CMSG_SPACE(buf_fds * sizeof(int)));
> +
> + if (send) {
> + msg.msg_control = fd_buf;
> + msg.msg_controllen = CMSG_LEN(buf_fds * sizeof(int));
> +
> + /* Set up fd_buf contents */
> + cmsg = CMSG_FIRSTHDR(&msg);
> + cmsg->cmsg_len = msg.msg_controllen;
> + cmsg->cmsg_level = SOL_SOCKET;
> + cmsg->cmsg_type = SCM_RIGHTS;
> + memcpy(CMSG_DATA(cmsg), *fds, buf_fds * sizeof(int));
> + }
> + }
> +
> + while (iovlen) {
> + memset(&msg, 0, sizeof(msg));
> + msg.msg_iov = iov;
> + msg.msg_iovlen = iovlen;
> + msg.msg_control = fd_buf;
> + msg.msg_controllen = fd_buf ?
> + CMSG_LEN(buf_fds * sizeof(int)) : 0;
> +
> + if (send)
> + r = sendmsg(fd, &msg, 0);
> + else
> + r = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC | MSG_WAITALL);
> +
> + if (r <= 0) {
Strictly speaking, you can't call sendmsg in a loop. The socket is
marked non-blocking, so EWOULDBLOCK or EAGAIN is a valid error return.
This means that you need to exit the loop and re-enter once POLLOUT is
received.
> + if (r != -1 || errno != EINTR)
> + return false;
> +
> + continue;
> + }
> +
> + while (iovlen && r >= (int) iov[0].iov_len) {
> + r -= iov[0].iov_len;
> + iov++;
> + iovlen--;
> + }
> +
> + if (iovlen) {
> + iov[0].iov_base += r;
> + iov[0].iov_len -= r;
> + }
> +
> + if (send)
> + continue;
> +
> + for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
> + cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> + int cnt;
> +
> + if (cmsg->cmsg_level != SOL_SOCKET ||
> + cmsg->cmsg_type != SCM_RIGHTS)
> + continue;
> +
> + cnt = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
> +
> + *fds = l_realloc(*fds, (*num_fds + cnt) * sizeof(int));
> +
> + memcpy(*fds + *num_fds, CMSG_DATA(cmsg),
> + cnt * sizeof(int));
> +
> + *num_fds += cnt;
> + }
> + }
> +
> + return true;
> +}
> +
> static bool classic_send_message(struct l_dbus *dbus,
> struct l_dbus_message *message)
> {
> int fd = l_io_get_fd(dbus->io);
> - struct msghdr msg;
> struct iovec iov[2];
> - ssize_t len;
> int *fds;
> uint32_t num_fds = 0;
> - struct cmsghdr *cmsg;
>
> iov[0].iov_base = _dbus_message_get_header(message, &iov[0].iov_len);
> iov[1].iov_base = _dbus_message_get_body(message, &iov[1].iov_len);
>
> - memset(&msg, 0, sizeof(msg));
> - msg.msg_iov = iov;
> - msg.msg_iovlen = 2;
> -
> if (dbus->support_unix_fd)
> fds = _dbus_message_get_fds(message, &num_fds);
>
> - if (num_fds) {
> - msg.msg_control = alloca(CMSG_SPACE(num_fds * sizeof(int)));
> - msg.msg_controllen = CMSG_LEN(num_fds * sizeof(int));
> -
> - cmsg = CMSG_FIRSTHDR(&msg);
> - cmsg->cmsg_len = msg.msg_controllen;
> - cmsg->cmsg_level = SOL_SOCKET;
> - cmsg->cmsg_type = SCM_RIGHTS;
> - memcpy(CMSG_DATA(cmsg), fds, num_fds * sizeof(int));
> - }
> -
> - len = sendmsg(fd, &msg, 0);
> - if (len < 0)
> - return false;
> -
> - return true;
> + return msg_op(fd, true, iov, &fds, &num_fds);
> }
>
> static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
> {
> int fd = l_io_get_fd(dbus->io);
> struct dbus_header hdr;
> - struct msghdr msg;
> struct iovec iov[2];
> - struct cmsghdr *cmsg;
> ssize_t len;
> void *header, *body;
> size_t header_size, body_size;
> - union {
> - uint8_t bytes[CMSG_SPACE(16 * sizeof(int))];
> - struct cmsghdr align;
> - } fd_buf;
> int *fds = NULL;
> uint32_t num_fds = 0;
> + struct l_dbus_message *msg;
> + unsigned int i;
>
> - len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
> + len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK | MSG_WAITALL);
> if (len != DBUS_HEADER_SIZE)
> return NULL;
>
> @@ -650,59 +713,37 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
> iov[1].iov_base = body;
> iov[1].iov_len = body_size;
>
> - memset(&msg, 0, sizeof(msg));
> - msg.msg_iov = iov;
> - msg.msg_iovlen = 2;
> - msg.msg_control = &fd_buf;
> - msg.msg_controllen = sizeof(fd_buf);
> -
> - len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
> - if (len < 0)
> - goto cmsg_fail;
> -
> - for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
> - cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> - unsigned int i;
> -
> - if (cmsg->cmsg_level != SOL_SOCKET ||
> - cmsg->cmsg_type != SCM_RIGHTS)
> - continue;
> -
> - num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
> - fds = (void *) CMSG_DATA(cmsg);
> -
> - /* Set FD_CLOEXEC on all file descriptors */
> - for (i = 0; i < num_fds; i++) {
> - long flags;
> -
> - flags = fcntl(fds[i], F_GETFD, NULL);
> - if (flags < 0)
> - continue;
> -
> - if (!(flags & FD_CLOEXEC))
> - fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
> - }
> - }
> + if (!msg_op(fd, false, iov, &fds, &num_fds))
> + goto fail;
>
> if (hdr.endian != DBUS_NATIVE_ENDIAN) {
> l_util_debug(dbus->debug_handler,
> dbus->debug_data, "Endianness incorrect");
> - goto bad_msg;
> + goto fail;
> }
>
> if (hdr.version != 1) {
> l_util_debug(dbus->debug_handler,
> dbus->debug_data, "Protocol version incorrect");
> - goto bad_msg;
> + goto fail;
> }
>
> - return dbus_message_build(header, header_size, body, body_size,
> + msg = dbus_message_build(header, header_size, body, body_size,
> fds, num_fds);
> + if (!msg)
> + goto fail;
>
> -bad_msg:
> -cmsg_fail:
> - l_free(header);
> + l_free(fds);
> +
> + return msg;
> +
> +fail:
> + for (i = 0; i < num_fds; i++)
> + close(fds[i]);
> +
> + l_free(fds);
> l_free(body);
> + l_free(header);
>
> return NULL;
> }
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] dbus: Close unused file descriptors when creating message
2016-05-04 11:03 [PATCH 1/4] dbus: Close unused file descriptors when creating message Andrew Zaborowski
` (2 preceding siblings ...)
2016-05-04 11:03 ` [PATCH 4/4] dbus: Handle partial reads and writes Andrew Zaborowski
@ 2016-05-04 16:05 ` Denis Kenzior
3 siblings, 0 replies; 9+ messages in thread
From: Denis Kenzior @ 2016-05-04 16:05 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 223 bytes --]
Hi Andrew,
On 05/04/2016 06:03 AM, Andrew Zaborowski wrote:
> ---
> ell/dbus-message.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
Applied, thanks.
Regards,
-Denis
^ permalink raw reply [flat|nested] 9+ messages in thread