[PATCH 1/2] dbus: Better search for matching rules in the filter tree

[PATCH 1/2] dbus: Better search for matching rules in the filter tree

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 4637 bytes --]

When adding new filter rules, perform a more complete search for
existing rules that already cover the rule being added by removing the
requierement that levels in the tree are ordered by the match type.
This may add overhead to adding new rules in some situations but will
reduce the number of server-side rules being used and the number of
requests.  This covers a scenario where first a watch is added for:

type=signal, path=/

followed by

type=signal, sender=org.test, path=/

I believe there are some more tricky scenarios which are not covered
specifically when more than one child of an existing node matches some
condition of the rule being added.
---
 ell/dbus-filter.c  | 67 +++++++++++++++++++++++++++++++++++++++++++-----------
 ell/dbus-private.h |  8 +++----
 2 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/ell/dbus-filter.c b/ell/dbus-filter.c
index 42755a9..9e3cad5 100644
--- a/ell/dbus-filter.c
+++ b/ell/dbus-filter.c
@@ -234,34 +234,63 @@ static bool remove_recurse(struct _dbus_filter *filter,
 }
 
 unsigned int _dbus_filter_add_rule(struct _dbus_filter *filter,
-					struct _dbus_filter_condition *rule,
-					int rule_len,
-					l_dbus_message_func_t signal_func,
-					void *user_data)
+				const struct _dbus_filter_condition *rule,
+				int rule_len,
+				l_dbus_message_func_t signal_func,
+				void *user_data)
 {
 	struct filter_node **node_ptr = &filter->root;
 	struct filter_node *node;
 	struct filter_node *parent = filter->root;
 	bool remote_rule = false;
-	struct _dbus_filter_condition *condition, *end = rule + rule_len;
-
-	qsort(rule, rule_len, sizeof(*rule), condition_compare);
-
-	for (condition = rule; condition < end; condition++) {
-		/* See if this condition is already a child of the node */
+	struct _dbus_filter_condition sorted[rule_len];
+	struct _dbus_filter_condition *unused, *condition;
+	struct _dbus_filter_condition *end = sorted + rule_len;
+
+	memcpy(sorted, rule, sizeof(sorted));
+	qsort(sorted, rule_len, sizeof(*condition), condition_compare);
+
+	/*
+	 * Find or create a path in the tree with a node for each
+	 * condition in the rule, loop until all conditions have been
+	 * used.
+	 */
+	unused = sorted;
+	while (unused < end) {
+		/*
+		 * Find a child of the node that matches any unused
+		 * condition.  Note there could be multiple matches, we're
+		 * happy with the first we can find.
+		 */
 		while (*node_ptr) {
 			node = *node_ptr;
 
-			if (node->type == condition->type &&
-					!strcmp(node->match.value,
+			for (condition = unused; condition < end; condition++) {
+				if (condition->type > node->type) {
+					condition = end;
+					break;
+				}
+
+				if (condition->type < node->type ||
+						condition->type ==
+						L_DBUS_MATCH_NONE)
+					continue;
+
+				if (!strcmp(node->match.value,
 						condition->value))
+					break;
+			}
+
+			if (condition < end)
 				break;
 
 			node_ptr = &node->next;
 		}
 
-		/* Add one */
+		/* Add a node */
 		if (!*node_ptr) {
+			condition = unused;
+
 			node = l_new(struct filter_node, 1);
 			node->type = condition->type;
 			node->match.value = l_strdup(condition->value);
@@ -278,6 +307,18 @@ unsigned int _dbus_filter_add_rule(struct _dbus_filter *filter,
 
 		}
 
+		/*
+		 * Mark the condition used.  We do this by setting
+		 * condition->type to an invalid value unless it is the
+		 * first condition left in which case we can push the
+		 * rule start.  Another option is to always push the rule
+		 * start and memmove the still unused conditions by one
+		 * if necessary.
+		 */
+		condition->type = L_DBUS_MATCH_NONE;
+		while (unused < end && unused[0].type == L_DBUS_MATCH_NONE)
+			unused++;
+
 		node_ptr = &node->match.children;
 
 		parent = node;
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index ab52895..f4209b8 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -320,10 +320,10 @@ struct _dbus_filter *_dbus_filter_new(struct l_dbus *dbus,
 void _dbus_filter_free(struct _dbus_filter *filter);
 
 unsigned int _dbus_filter_add_rule(struct _dbus_filter *filter,
-					struct _dbus_filter_condition *rule,
-					int rule_len,
-					l_dbus_message_func_t signal_func,
-					void *user_data);
+				const struct _dbus_filter_condition *rule,
+				int rule_len,
+				l_dbus_message_func_t signal_func,
+				void *user_data);
 bool _dbus_filter_remove_rule(struct _dbus_filter *filter, unsigned int id);
 
 char *_dbus_filter_rule_to_str(const struct _dbus_filter_condition *rule,
-- 
2.5.0

[PATCH 2/2] unit: test a filter tree scenario with unordered paths

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 4592 bytes --]

Test a scenario in which some paths in the filter tree are not ordered
by the match type.
---
 unit/test-dbus-watch.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/unit/test-dbus-watch.c b/unit/test-dbus-watch.c
index c7e78a5..46acf06 100644
--- a/unit/test-dbus-watch.c
+++ b/unit/test-dbus-watch.c
@@ -289,7 +289,7 @@ struct filter_test_state {
 	const struct _dbus_filter_condition *expected_rule;
 	int expected_rule_len;
 	unsigned int expected_id, new_id;
-	int calls[4];
+	int calls[5];
 };
 
 static void rule_compare(const struct _dbus_filter_condition *a, int len_a,
@@ -372,6 +372,13 @@ static void test_rule4_cb(struct l_dbus_message *message, void *user_data)
 	test->calls[3]++;
 }
 
+static void test_rule5_cb(struct l_dbus_message *message, void *user_data)
+{
+	struct filter_test_state *test = user_data;
+
+	test->calls[4]++;
+}
+
 static void test_filter_tree(const void *test_data)
 {
 	struct _dbus_filter *filter;
@@ -381,7 +388,7 @@ static void test_filter_tree(const void *test_data)
 		.add_match = test_add_match,
 		.remove_match = test_remove_match,
 	};
-	static struct _dbus_filter_condition rule123[] = {
+	static const struct _dbus_filter_condition rule123[] = {
 		{ L_DBUS_MATCH_TYPE, "signal" },
 		{ L_DBUS_MATCH_SENDER, DBUS_SERVICE_DBUS },
 		{ L_DBUS_MATCH_PATH, DBUS_PATH_DBUS },
@@ -389,11 +396,12 @@ static void test_filter_tree(const void *test_data)
 		{ L_DBUS_MATCH_MEMBER, "NameOwnerChanged" },
 		{ L_DBUS_MATCH_ARGUMENT(0), "org.test" }
 	};
-	static struct _dbus_filter_condition rule4[] = {
+	static const struct _dbus_filter_condition rule45[] = {
 		{ L_DBUS_MATCH_TYPE, "signal" },
-		{ L_DBUS_MATCH_SENDER, "foo" },
+		{ L_DBUS_MATCH_PATH, "/" },
+		{ L_DBUS_MATCH_SENDER, "org.foo" },
 	};
-	unsigned int id1, id2, id3, id4, internal_id1, internal_id4;
+	unsigned int id1, id2, id3, id4, id5, internal_id1, internal_id4;
 	struct l_dbus_message *message;
 
 	filter = _dbus_filter_new(&test.dbus, &filter_ops, NULL);
@@ -410,15 +418,19 @@ static void test_filter_tree(const void *test_data)
 	id3 = _dbus_filter_add_rule(filter, rule123, 6, test_rule3_cb, &test);
 	assert(id2 && id3 && id2 != id1 && id3 != id1 && id3 != id2);
 
-	test.expected_rule = rule4;
+	test.expected_rule = rule45;
 	test.expected_rule_len = 2;
-	id4 = _dbus_filter_add_rule(filter, rule4, 2, test_rule4_cb, &test);
+	id4 = _dbus_filter_add_rule(filter, rule45, 2, test_rule4_cb, &test);
 	assert(id4 && id4 != id1 && id4 != id2 && id4 != id3);
 	assert(!test.expected_rule);
 	internal_id4 = test.new_id;
 
+	id5 = _dbus_filter_add_rule(filter, rule45, 3, test_rule5_cb, &test);
+	assert(id5 && id5 != id1 && id5 != id2 && id5 != id3 && id5 != id4);
+
 	assert(test.calls[0] == 0 && test.calls[1] == 0 &&
-			test.calls[2] == 0 && test.calls[3] == 0);
+			test.calls[2] == 0 && test.calls[3] == 0 &&
+			test.calls[4] == 0);
 
 	message = _dbus_message_new_signal(2, DBUS_PATH_DBUS,
 						DBUS_INTERFACE_DBUS,
@@ -441,22 +453,28 @@ static void test_filter_tree(const void *test_data)
 	l_dbus_message_set_arguments(message, "");
 	_dbus_message_set_sender(message, DBUS_SERVICE_DBUS);
 	_dbus_filter_dispatch(message, filter);
+	l_dbus_message_unref(message);
 
-	_dbus_message_set_sender(message, "foo");
+	message = _dbus_message_new_signal(2, "/", "foo", "Bar");
+	l_dbus_message_set_arguments(message, "");
+	_dbus_message_set_sender(message, "org.foo");
 	_dbus_filter_dispatch(message, filter);
 
-	_dbus_message_set_sender(message, "bar");
+	_dbus_message_set_sender(message, "org.bar");
 	_dbus_filter_dispatch(message, filter);
 	l_dbus_message_unref(message);
 
 	assert(test.calls[0] == 3 && test.calls[1] == 2 &&
-			test.calls[2] == 1 && test.calls[3] == 1);
+			test.calls[2] == 1 && test.calls[3] == 2 &&
+			test.calls[4] == 1);
 
 	test.expected_id = 0;
 	assert(_dbus_filter_remove_rule(filter, id2));
 
 	assert(_dbus_filter_remove_rule(filter, id1));
 
+	assert(_dbus_filter_remove_rule(filter, id5));
+
 	test.expected_id = internal_id4;
 	assert(_dbus_filter_remove_rule(filter, id4));
 	assert(!test.expected_id);
@@ -468,7 +486,8 @@ static void test_filter_tree(const void *test_data)
 	_dbus_filter_free(filter);
 
 	assert(test.calls[0] == 3 && test.calls[1] == 2 &&
-			test.calls[2] == 1 && test.calls[3] == 1);
+			test.calls[2] == 1 && test.calls[3] == 2 &&
+			test.calls[4] == 1);
 }
 
 int main(int argc, char *argv[])
-- 
2.5.0

[PATCH 1/2] dbus: Fix Dbus1 parsing of doubles

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

Don't decode the 8-byte value as uint64_t and then cast to double
because that will produce a different number.  Gvariant already does
this right.
---
 ell/dbus-util.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/ell/dbus-util.c b/ell/dbus-util.c
index 6414216..1298228 100644
--- a/ell/dbus-util.c
+++ b/ell/dbus-util.c
@@ -675,17 +675,11 @@ bool _dbus1_iter_next_entry_basic(struct l_dbus_message_iter *iter,
 		iter->pos = pos + 8;
 		break;
 	case 't':
-		if (pos + 8 > iter->len)
-			return false;
-		uint64_val = get_u64(iter->data + pos);
-		*(uint64_t *) out = uint64_val;
-		iter->pos = pos + 8;
-		break;
 	case 'd':
 		if (pos + 8 > iter->len)
 			return false;
 		uint64_val = get_u64(iter->data + pos);
-		*(double *) out = (double) uint64_val;
+		*(uint64_t *) out = uint64_val;
 		iter->pos = pos + 8;
 		break;
 	default:
-- 
2.5.0

[PATCH 2/2] unit: Fix typos that make the assertions always true

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

---
 unit/test-dbus-message.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/unit/test-dbus-message.c b/unit/test-dbus-message.c
index ce58769..c8e2059 100644
--- a/unit/test-dbus-message.c
+++ b/unit/test-dbus-message.c
@@ -1501,7 +1501,7 @@ static void parse_basic_5(const void *data)
 
 	result = l_dbus_message_get_arguments(msg, "q", &val);
 	assert(result);
-	assert(val = 42);
+	assert(val == 42);
 
 	l_dbus_message_unref(msg);
 }
@@ -1524,7 +1524,7 @@ static void parse_basic_6(const void *data)
 
 	result = l_dbus_message_get_arguments(msg, "u", &val);
 	assert(result);
-	assert(val = 4711);
+	assert(val == 4711);
 
 	l_dbus_message_unref(msg);
 }
@@ -1546,7 +1546,7 @@ static void parse_basic_7(const void *data)
 
 	result = l_dbus_message_get_arguments(msg, "t", &val);
 	assert(result);
-	assert(val = 10000);
+	assert(val == 10000);
 
 	l_dbus_message_unref(msg);
 }
@@ -1614,7 +1614,7 @@ static void parse_basic_10(const void *data)
 
 	result = l_dbus_message_get_arguments(msg, "x", &val);
 	assert(result);
-	assert(val = -10000);
+	assert(val == -10000);
 
 	l_dbus_message_unref(msg);
 }
@@ -1637,7 +1637,7 @@ static void parse_basic_11(const void *data)
 
 	result = l_dbus_message_get_arguments(msg, "d", &val);
 	assert(result);
-	assert(val = M_PI);
+	assert(val == M_PI);
 
 	l_dbus_message_unref(msg);
 }
-- 
2.5.0

[PATCH] dbus: Don't use KDBUS_ATTACH_NAMES for kdbus connection

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

On the receive side we're not using the list of names being attached to
messages because we keep a cache of the name owners we need.  On the
send side the KDBUS_ATTACH_NAMES is implicitly granted so there's no
need to specify it.
---
 ell/dbus-kernel.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 4fd2f29..50ee0ed 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -248,8 +248,6 @@ int _dbus_kernel_hello(int fd, const char *connection_name,
 
 	hello->size = size;
 	hello->flags |= KDBUS_HELLO_ACCEPT_FD;
-	hello->attach_flags_send |= KDBUS_ATTACH_NAMES;
-	hello->attach_flags_recv |= KDBUS_ATTACH_NAMES;
 	hello->pool_size = KDBUS_POOL_SIZE;
 
 	item = hello->items;
-- 
2.5.0

[PATCH 01/11] dbus: Accept a list of FDs in dbus_message_from_blob

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2613 bytes --]

---
 ell/dbus-kernel.c  |  2 +-
 ell/dbus-message.c | 16 ++++++++++++++--
 ell/dbus-private.h |  3 ++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 50ee0ed..7446ece 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -516,7 +516,7 @@ static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 
 	collect_body_parts(kmsg, size, &data);
 
-	*out_message = dbus_message_from_blob(data, size);
+	*out_message = dbus_message_from_blob(data, size, NULL, 0);
 
 	l_free(data);
 
diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index f3d82d8..9fe2a59 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -691,11 +691,13 @@ static bool valid_header(const struct dbus_header *hdr)
 	return true;
 }
 
-struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
+struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size,
+						int fds[], uint32_t num_fds)
 {
 	const struct dbus_header *hdr = data;
 	struct l_dbus_message *message;
 	size_t body_pos;
+	unsigned int i;
 
 	if (unlikely(size < DBUS_HEADER_SIZE))
 		return NULL;
@@ -759,6 +761,16 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
 		get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
 					'g', &message->signature);
 
+	if (num_fds > L_ARRAY_SIZE(message->fds)) {
+		for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
+			close(fds[i]);
+
+		num_fds = L_ARRAY_SIZE(message->fds);
+	}
+
+	message->num_fds = num_fds;
+	memcpy(message->fds, fds, num_fds * sizeof(int));
+
 	return message;
 
 free:
@@ -812,7 +824,7 @@ bool dbus_message_compare(struct l_dbus_message *message,
 	struct l_dbus_message *other;
 	bool ret = false;
 
-	other = dbus_message_from_blob(data, size);
+	other = dbus_message_from_blob(data, size, NULL, 0);
 
 	if (message->signature) {
 		if (!other->signature)
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index f4209b8..e8dbce7 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -147,7 +147,8 @@ struct l_dbus_message *_dbus_message_new_error(uint8_t version,
 						const char *name,
 						const char *error);
 
-struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size);
+struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size,
+						int fds[], uint32_t num_fds);
 struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 						void *body, size_t body_size,
 						int fds[], uint32_t num_fds);
-- 
2.5.0

[PATCH 02/11] unit: Update dbus tests to use new dbus_message_from_blob prototype

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

---
 unit/test-dbus-message.c     | 4 +++-
 unit/test-gvariant-message.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/unit/test-dbus-message.c b/unit/test-dbus-message.c
index c8e2059..20b09bc 100644
--- a/unit/test-dbus-message.c
+++ b/unit/test-dbus-message.c
@@ -48,6 +48,7 @@ struct message_data {
 	uint32_t unix_fds;
 	const unsigned char *binary;
 	size_t binary_len;
+	int *fds;
 };
 
 static const unsigned char message_binary_basic_1[] = {
@@ -1320,7 +1321,8 @@ static struct l_dbus_message *check_message(const struct message_data *msg_data)
 {
 	struct l_dbus_message *msg;
 
-	msg = dbus_message_from_blob(msg_data->binary, msg_data->binary_len);
+	msg = dbus_message_from_blob(msg_data->binary, msg_data->binary_len,
+					msg_data->fds, msg_data->unix_fds);
 	assert(msg);
 
 	if (do_print)
diff --git a/unit/test-gvariant-message.c b/unit/test-gvariant-message.c
index 85326a6..8ded4cd 100644
--- a/unit/test-gvariant-message.c
+++ b/unit/test-gvariant-message.c
@@ -138,7 +138,8 @@ static struct l_dbus_message *check_message(const struct message_data *msg_data)
 {
 	struct l_dbus_message *msg;
 
-	msg = dbus_message_from_blob(msg_data->binary, msg_data->binary_len);
+	msg = dbus_message_from_blob(msg_data->binary, msg_data->binary_len,
+					NULL, 0);
 	assert(msg);
 
 	if (msg_data->path) {
-- 
2.5.0

[PATCH 03/11] dbus: Validate the size of fd list in dbus_message_build

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

Trim the list of fds if too big rather than error out to be consistent
with how we handle invalid FDs in message_iter_next_entry_valist and
how kdbus allows messages with invalid FDs to still be delivered.
---
 ell/dbus-message.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 9fe2a59..8151c69 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -784,6 +784,7 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 {
 	const struct dbus_header *hdr = header;
 	struct l_dbus_message *message;
+	unsigned int i;
 
 	if (unlikely(header_size < DBUS_HEADER_SIZE))
 		return NULL;
@@ -806,6 +807,13 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 	message->body_size = body_size;
 	message->body = body;
 
+	if (num_fds > L_ARRAY_SIZE(message->fds)) {
+		for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
+			close(fds[i]);
+
+		num_fds = L_ARRAY_SIZE(message->fds);
+	}
+
 	message->num_fds = num_fds;
 	memcpy(message->fds, fds, num_fds * sizeof(int));
 
-- 
2.5.0

[PATCH 04/11] dbus: Handle the 'y' type in append_arguments

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

FD values can't be handled same as int32 values ('i' and 'u').
---
 ell/dbus-message.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 8151c69..5f91360 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -1137,7 +1137,6 @@ static bool append_arguments(struct l_dbus_message *message,
 		}
 		case 'i':
 		case 'u':
-		case 'h':
 		{
 			uint32_t u = va_arg(args, uint32_t);
 
@@ -1146,6 +1145,20 @@ static bool append_arguments(struct l_dbus_message *message,
 
 			break;
 		}
+		case 'h':
+		{
+			int fd = va_arg(args, int);
+
+			if (!driver->append_basic(builder, *s,
+							&message->num_fds))
+				goto error;
+
+			if (message->num_fds < L_ARRAY_SIZE(message->fds))
+				message->fds[message->num_fds++] =
+					fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
+
+			break;
+		}
 		case 'x':
 		case 't':
 		{
-- 
2.5.0

[PATCH 05/11] dbus: Check the FD array size in l_dbus_message_builder_append_from_iter

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

Also make sure an invalid FD in the input message is reflected as an
invalid FD in the output message and non-fatal.
---
 ell/dbus-message.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 5f91360..628f994 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -1862,7 +1862,6 @@ LIB_EXPORT bool l_dbus_message_builder_append_from_iter(
 	void *basic_ptr;
 	uint64_t basic;
 	uint32_t uint32_val;
-	int fd;
 	bool (*get_basic)(struct l_dbus_message_iter *, char, void *);
 	bool (*enter_func)(struct l_dbus_message_iter *,
 				struct l_dbus_message_iter *);
@@ -1908,17 +1907,22 @@ LIB_EXPORT bool l_dbus_message_builder_append_from_iter(
 		if (!get_basic(from, type, &uint32_val))
 			return false;
 
-		if (uint32_val >= from->message->num_fds)
+		if (!l_dbus_message_builder_append_basic(builder, type,
+						&builder->message->num_fds))
 			return false;
 
-		fd = fcntl(from->message->fds[uint32_val], F_DUPFD_CLOEXEC, 3);
+		if (builder->message->num_fds <
+				L_ARRAY_SIZE(builder->message->fds)) {
+			int fd;
 
-		uint32_val = builder->message->num_fds;
-		builder->message->fds[builder->message->num_fds++] = fd;
+			if (uint32_val < from->message->num_fds)
+				fd = fcntl(from->message->fds[uint32_val],
+						F_DUPFD_CLOEXEC, 3);
+			else
+				fd = -1;
 
-		if (!l_dbus_message_builder_append_basic(builder, type,
-								&uint32_val))
-			return false;
+			builder->message->fds[builder->message->num_fds++] = fd;
+		}
 
 		return true;
 	case '(':
-- 
2.5.0

[PATCH 06/11] dbus: Add utility to get FD array for a message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

---
 ell/dbus-message.c | 7 +++++++
 ell/dbus-private.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 628f994..755dcf5 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -126,6 +126,13 @@ void *_dbus_message_get_footer(struct l_dbus_message *msg, size_t *out_size)
 	return msg->body;
 }
 
+int *_dbus_message_get_fds(struct l_dbus_message *msg, uint32_t *num_fds)
+{
+	*num_fds = msg->num_fds;
+
+	return msg->fds;
+}
+
 void _dbus_message_set_serial(struct l_dbus_message *msg, uint32_t serial)
 {
 	struct dbus_header *hdr = msg->header;
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index e8dbce7..88bea3c 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -116,6 +116,7 @@ bool _dbus1_builder_rewind(struct dbus_builder *builder);
 void *_dbus_message_get_body(struct l_dbus_message *msg, size_t *out_size);
 void *_dbus_message_get_header(struct l_dbus_message *msg, size_t *out_size);
 void *_dbus_message_get_footer(struct l_dbus_message *msg, size_t *out_size);
+int *_dbus_message_get_fds(struct l_dbus_message *msg, uint32_t *num_fds);
 void _dbus_message_set_serial(struct l_dbus_message *msg, uint32_t serial);
 uint32_t _dbus_message_get_serial(struct l_dbus_message *msg);
 uint32_t _dbus_message_get_reply_serial(struct l_dbus_message *message);
-- 
2.5.0

[PATCH 07/11] dbus: Attach FDs to Dbus1 messages being sent

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

---
 ell/dbus.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/ell/dbus.c b/ell/dbus.c
index af0000d..70da849 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -596,6 +596,9 @@ static bool classic_send_message(struct l_dbus *dbus,
 	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);
@@ -604,6 +607,20 @@ static bool classic_send_message(struct l_dbus *dbus,
 	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;
-- 
2.5.0

[PATCH 08/11] dbus: Remove memcpy and fix setting of FD_CLOEXEC on FDs

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

Make sure the size of the FD buffer passed to recvmsg is properly
aligned, etc. and remove the memcpy which seems unneeded.  Also it looks
like the fcntl() calls operated on one dbus socket FD instead of the
message FDs.
---
 ell/dbus.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/ell/dbus.c b/ell/dbus.c
index 70da849..fc0f243 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -638,7 +638,8 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 	ssize_t len;
 	void *header, *body;
 	size_t header_size, body_size;
-	int fds[16];
+	uint8_t fd_buf[CMSG_SPACE(16 * sizeof(int))];
+	int *fds = NULL;
 	uint32_t num_fds = 0;
 
 	len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
@@ -659,8 +660,8 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 	memset(&msg, 0, sizeof(msg));
 	msg.msg_iov = iov;
 	msg.msg_iovlen = 2;
-	msg.msg_control = &fds;
-	msg.msg_controllen = CMSG_SPACE(16 * sizeof(int));
+	msg.msg_control = &fd_buf;
+	msg.msg_controllen = sizeof(fd_buf);
 
 	len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
 	if (len < 0)
@@ -675,19 +676,18 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 			continue;
 
 		num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
-
-		memcpy(fds, CMSG_DATA(cmsg), num_fds * 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(fd, F_GETFD, NULL);
+			flags = fcntl(fds[i], F_GETFD, NULL);
 			if (flags < 0)
 				continue;
 
 			if (!(flags & FD_CLOEXEC))
-				fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+				fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
                 }
 	}
 
-- 
2.5.0

[PATCH 09/11] dbus: Attach FDs to kdbus messages being sent

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

---
 ell/dbus-kernel.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 7446ece..302e3b9 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -325,6 +325,8 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
 	size_t footer_size;
 	int ret;
 	struct kdbus_cmd_send cmd;
+	uint32_t num_fds;
+	int *fds;
 	L_AUTO_FREE_VAR(struct kdbus_msg *, kmsg);
 
 	dest = l_dbus_message_get_destination(message);
@@ -350,6 +352,11 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
 	if (dest && !unique)
 		kmsg_size += KDBUS_ITEM_SIZE(dest_len + 1);
 
+	/* Reserve space for fds */
+	fds = _dbus_message_get_fds(message, &num_fds);
+	if (num_fds)
+		kmsg_size += KDBUS_ITEM_SIZE(num_fds * sizeof(int));
+
 	kmsg = aligned_alloc(8, kmsg_size);
 	if (!kmsg)
 		return -ENOMEM;
@@ -433,6 +440,13 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
 	item->vec.size = footer_size;
 	item = KDBUS_ITEM_NEXT(item);
 
+	if (num_fds) {
+		item->size = KDBUS_ITEM_HEADER_SIZE + num_fds * sizeof(int);
+		item->type = KDBUS_ITEM_FDS;
+		memcpy(item->fds, fds, num_fds * sizeof(int));
+		item = KDBUS_ITEM_NEXT(item);
+	}
+
 	kmsg->size = (void *)item - (void *)kmsg;
 
 	memset(&cmd, 0, sizeof(cmd));
-- 
2.5.0

[PATCH 10/11] dbus: Handle received KDBUS_ITEM_FDS

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

---
 ell/dbus-kernel.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 302e3b9..2cd1d94 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -489,6 +489,9 @@ static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 	struct dbus_header *hdr;
 	const char *destination = 0;
 	char unique_bus_name[128];
+	uint32_t num_fds = 0;
+	int *fds = NULL;
+	unsigned int i;
 
 	KDBUS_ITEM_FOREACH(item, kmsg, items) {
 		switch (item->type) {
@@ -509,7 +512,28 @@ static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 
 			return -ENOTSUP;
 		case KDBUS_ITEM_FDS:
-			return -ENOTSUP;
+			if (fds || item->size <= KDBUS_ITEM_HEADER_SIZE)
+				return -EBADMSG;
+
+			num_fds = (item->size - KDBUS_ITEM_HEADER_SIZE) /
+				sizeof(int);
+			fds = item->fds;
+
+			/* Set FD_CLOEXEC on all file descriptors */
+			for (i = 0; i < num_fds; i++) {
+				long flags;
+
+				if (fds[i] == -1)
+					continue;
+
+				flags = fcntl(fds[i], F_GETFD, NULL);
+				if (flags < 0 || (flags & FD_CLOEXEC))
+					continue;
+
+				fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
+			}
+
+			break;
 		case KDBUS_ITEM_DST_NAME:
 			if (!_dbus_valid_bus_name(item->str))
 				return -EBADMSG;
@@ -530,7 +554,7 @@ static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 
 	collect_body_parts(kmsg, size, &data);
 
-	*out_message = dbus_message_from_blob(data, size, NULL, 0);
+	*out_message = dbus_message_from_blob(data, size, fds, num_fds);
 
 	l_free(data);
 
-- 
2.5.0

[PATCH 11/11] unit: 'h' type parsing and building tests.

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3931 bytes --]

---
 unit/test-dbus-message.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/unit/test-dbus-message.c b/unit/test-dbus-message.c
index 20b09bc..52df9d8 100644
--- a/unit/test-dbus-message.c
+++ b/unit/test-dbus-message.c
@@ -26,6 +26,10 @@
 
 #include <assert.h>
 #include <math.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
 
 #include <ell/ell.h>
 
@@ -2578,6 +2582,124 @@ static void builder_rewind(const void *data)
 	compare_message(msg, data);
 }
 
+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 void compare_files(int a, int b)
+{
+	struct stat sa, sb;
+
+	assert(fstat(a, &sa) == 0);
+	assert(fstat(b, &sb) == 0);
+
+	assert(sa.st_dev == sb.st_dev);
+	assert(sa.st_ino == sb.st_ino);
+	assert(sa.st_rdev == sb.st_rdev);
+}
+
+static const unsigned char message_binary_basic_h[] = {
+	0x6c, 0x01, 0x00, 0x01, 0x04, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x6f, 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,
+	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,
+	0x08, 0x01, 0x67, 0x00, 0x01, 0x68, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+};
+
+static const struct message_data message_data_basic_h = {
+	.type		= "method_call",
+	.path		= "/com/example/object",
+	.interface	= "com.example.interface",
+	.member		= "method",
+	.destination	= "com.example",
+	.signature	= "h",
+	.serial		= 0,
+	.reply_serial	= 0,
+	.no_reply	= 0,
+	.auto_start	= 1,
+	.unix_fds	= 1,
+	.binary		= message_binary_basic_h,
+	.binary_len	= sizeof(message_binary_basic_h),
+};
+
+static void message_fds_parse(const void *data)
+{
+	struct message_data message_data = message_data_basic_h;
+	struct l_dbus_message *msg;
+	int fd0, fd1;
+	int open_fds;
+	bool result;
+
+	open_fds = count_fds();
+
+	fd0 = open("/dev/random", O_RDONLY);
+	assert(fd0 != -1);
+
+	message_data.fds = &fd0;
+	msg = check_message(&message_data);
+
+	result = l_dbus_message_get_arguments(msg, "h", &fd1);
+	assert(result);
+	assert(fd1 != -1 && fd0 != fd1);
+
+	compare_files(fd0, fd1);
+
+	assert(count_fds() != open_fds);
+
+	close(fd1);
+
+	assert(count_fds() != open_fds);
+
+	l_dbus_message_unref(msg);
+
+	assert(count_fds() == open_fds);
+}
+
+static void message_fds_build(const void *data)
+{
+	struct message_data message_data = message_data_basic_h;
+	struct l_dbus_message *msg;
+	int fd;
+	int open_fds;
+
+	open_fds = count_fds();
+
+	fd = open("/dev/random", O_RDONLY);
+	assert(fd != -1);
+
+	msg = build_message(&message_data);
+
+	l_dbus_message_set_arguments(msg, "h", fd);
+
+	close(fd);
+
+	assert(count_fds() != open_fds);
+
+	compare_message(msg, &message_data);
+
+	assert(count_fds() == open_fds);
+}
+
 int main(int argc, char *argv[])
 {
 	l_test_init(&argc, &argv);
@@ -2674,5 +2796,8 @@ int main(int argc, char *argv[])
 	l_test_add("Message Builder Rewind Complex 1", builder_rewind,
 						&message_data_complex_1);
 
+	l_test_add("FDs (parse)", message_fds_parse, NULL);
+	l_test_add("FDs (build)", message_fds_build, NULL);
+
 	return l_test_run();
 }
-- 
2.5.0

Re: [PATCH 1/2] dbus: Fix Dbus1 parsing of doubles

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

Hi Andrew,

On 04/28/2016 10:31 PM, Andrew Zaborowski wrote:
> Don't decode the 8-byte value as uint64_t and then cast to double
> because that will produce a different number.  Gvariant already does
> this right.
> ---
>   ell/dbus-util.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/2] unit: Fix typos that make the assertions always true

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]

Hi Andrew,

On 04/28/2016 10:31 PM, Andrew Zaborowski wrote:
> ---
>   unit/test-dbus-message.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH] dbus: Don't use KDBUS_ATTACH_NAMES for kdbus connection

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

Hi Andrew,

On 04/28/2016 10:31 PM, Andrew Zaborowski wrote:
> On the receive side we're not using the list of names being attached to
> messages because we keep a cache of the name owners we need.  On the
> send side the KDBUS_ATTACH_NAMES is implicitly granted so there's no
> need to specify it.
> ---
>   ell/dbus-kernel.c | 2 --
>   1 file changed, 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 01/11] dbus: Accept a list of FDs in dbus_message_from_blob

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]

Hi Andrew,

On 04/28/2016 10:31 PM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-kernel.c  |  2 +-
>   ell/dbus-message.c | 16 ++++++++++++++--
>   ell/dbus-private.h |  3 ++-
>   3 files changed, 17 insertions(+), 4 deletions(-)

Applied, thanks.

Regards,
-Denis

Re: [PATCH 02/11] unit: Update dbus tests to use new dbus_message_from_blob prototype

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]

Hi Andrew,

On 04/28/2016 10:31 PM, Andrew Zaborowski wrote:
> ---
>   unit/test-dbus-message.c     | 4 +++-
>   unit/test-gvariant-message.c | 3 ++-
>   2 files changed, 5 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 03/11] dbus: Validate the size of fd list in dbus_message_build

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> Trim the list of fds if too big rather than error out to be consistent
> with how we handle invalid FDs in message_iter_next_entry_valist and
> how kdbus allows messages with invalid FDs to still be delivered.
> ---
>   ell/dbus-message.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 04/11] dbus: Handle the 'y' type in append_arguments

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> FD values can't be handled same as int32 values ('i' and 'u').
> ---
>   ell/dbus-message.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
> index 8151c69..5f91360 100644
> --- a/ell/dbus-message.c
> +++ b/ell/dbus-message.c
> @@ -1137,7 +1137,6 @@ static bool append_arguments(struct l_dbus_message *message,
>   		}
>   		case 'i':
>   		case 'u':
> -		case 'h':
>   		{
>   			uint32_t u = va_arg(args, uint32_t);
>
> @@ -1146,6 +1145,20 @@ static bool append_arguments(struct l_dbus_message *message,
>
>   			break;
>   		}
> +		case 'h':
> +		{
> +			int fd = va_arg(args, int);
> +
> +			if (!driver->append_basic(builder, *s,
> +							&message->num_fds))
> +				goto error;
> +
> +			if (message->num_fds < L_ARRAY_SIZE(message->fds))
> +				message->fds[message->num_fds++] =
> +					fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> +

This compound assignment is a bit weird, especially given that fd is not 
used afterward.  Can we simply assign
message->fds[...] = fcntl?

Or:
fd = fcntl(...);
message->fds[...] = fd;

> +			break;
> +		}
>   		case 'x':
>   		case 't':
>   		{
>

Regards,
-Denis

Re: [PATCH 05/11] dbus: Check the FD array size in l_dbus_message_builder_append_from_iter

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> Also make sure an invalid FD in the input message is reflected as an
> invalid FD in the output message and non-fatal.
> ---
>   ell/dbus-message.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 06/11] dbus: Add utility to get FD array for a message

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 220 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-message.c | 7 +++++++
>   ell/dbus-private.h | 1 +
>   2 files changed, 8 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 07/11] dbus: Attach FDs to Dbus1 messages being sent

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 193 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> ---
>   ell/dbus.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 08/11] dbus: Remove memcpy and fix setting of FD_CLOEXEC on FDs

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> Make sure the size of the FD buffer passed to recvmsg is properly
> aligned, etc. and remove the memcpy which seems unneeded.  Also it looks
> like the fcntl() calls operated on one dbus socket FD instead of the
> message FDs.
> ---
>   ell/dbus.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index 70da849..fc0f243 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -638,7 +638,8 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
>   	ssize_t len;
>   	void *header, *body;
>   	size_t header_size, body_size;
> -	int fds[16];
> +	uint8_t fd_buf[CMSG_SPACE(16 * sizeof(int))];

'man cmsg' recommends doing:
            union {
                /* ancillary data buffer, wrapped in a union in order to 
ensure
                   it is suitably aligned */
                char buf[CMSG_SPACE(sizeof myfds)];
                struct cmsghdr align;
            } u;

And it looks like systemd does the same.  This is probably what we 
should do as well.

> +	int *fds = NULL;
>   	uint32_t num_fds = 0;
>
>   	len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
> @@ -659,8 +660,8 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
>   	memset(&msg, 0, sizeof(msg));
>   	msg.msg_iov = iov;
>   	msg.msg_iovlen = 2;
> -	msg.msg_control = &fds;
> -	msg.msg_controllen = CMSG_SPACE(16 * sizeof(int));
> +	msg.msg_control = &fd_buf;
> +	msg.msg_controllen = sizeof(fd_buf);
>
>   	len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
>   	if (len < 0)
> @@ -675,19 +676,18 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
>   			continue;
>
>   		num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
> -
> -		memcpy(fds, CMSG_DATA(cmsg), num_fds * 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(fd, F_GETFD, NULL);
> +			flags = fcntl(fds[i], F_GETFD, NULL);
>   			if (flags < 0)
>   				continue;
>
>   			if (!(flags & FD_CLOEXEC))
> -				fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
> +				fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
>                   }
>   	}
>
>

Regards,
-Denis

Re: [PATCH 09/11] dbus: Attach FDs to kdbus messages being sent

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 197 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-kernel.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 10/11] dbus: Handle received KDBUS_ITEM_FDS

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 227 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-kernel.c | 28 ++++++++++++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 11/11] unit: 'h' type parsing and building tests.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> ---
>   unit/test-dbus-message.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 125 insertions(+)
>

Went ahead and applied this one.  It fails since patch 4 and 8 are still 
missing, so resubmit those soonish please.

Regards,
-Denis

Re: [PATCH 04/11] dbus: Handle the 'y' type in append_arguments

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

Hi Denis,

On 29 April 2016 at 17:50, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
>>
>> FD values can't be handled same as int32 values ('i' and 'u').
>> ---
>>   ell/dbus-message.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
>> index 8151c69..5f91360 100644
>> --- a/ell/dbus-message.c
>> +++ b/ell/dbus-message.c
>> @@ -1137,7 +1137,6 @@ static bool append_arguments(struct l_dbus_message
>> *message,
>>                 }
>>                 case 'i':
>>                 case 'u':
>> -               case 'h':
>>                 {
>>                         uint32_t u = va_arg(args, uint32_t);
>>
>> @@ -1146,6 +1145,20 @@ static bool append_arguments(struct l_dbus_message
>> *message,
>>
>>                         break;
>>                 }
>> +               case 'h':
>> +               {
>> +                       int fd = va_arg(args, int);
>> +
>> +                       if (!driver->append_basic(builder, *s,
>> +
>> &message->num_fds))
>> +                               goto error;
>> +
>> +                       if (message->num_fds < L_ARRAY_SIZE(message->fds))
>> +                               message->fds[message->num_fds++] =
>> +                                       fd = fcntl(fd, F_DUPFD_CLOEXEC,
>> 3);
>> +
>
>
> This compound assignment is a bit weird, especially given that fd is not
> used afterward.  Can we simply assign
> message->fds[...] = fcntl?

Oops sure, this was a typo I think.

Best regards

Re: [PATCH 08/11] dbus: Remove memcpy and fix setting of FD_CLOEXEC on FDs

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

Hi Denis,

On 29 April 2016 at 18:11, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
>>
>> Make sure the size of the FD buffer passed to recvmsg is properly
>> aligned, etc. and remove the memcpy which seems unneeded.  Also it looks
>> like the fcntl() calls operated on one dbus socket FD instead of the
>> message FDs.
>> ---
>>   ell/dbus.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/ell/dbus.c b/ell/dbus.c
>> index 70da849..fc0f243 100644
>> --- a/ell/dbus.c
>> +++ b/ell/dbus.c
>> @@ -638,7 +638,8 @@ static struct l_dbus_message
>> *classic_recv_message(struct l_dbus *dbus)
>>         ssize_t len;
>>         void *header, *body;
>>         size_t header_size, body_size;
>> -       int fds[16];
>> +       uint8_t fd_buf[CMSG_SPACE(16 * sizeof(int))];
>
>
> 'man cmsg' recommends doing:
>            union {
>                /* ancillary data buffer, wrapped in a union in order to
> ensure
>                   it is suitably aligned */
>                char buf[CMSG_SPACE(sizeof myfds)];
>                struct cmsghdr align;
>            } u;
>
> And it looks like systemd does the same.  This is probably what we should do
> as well.

Ah yes, I didn't think about the alignment, will fix and resend.  BTW
I my unit test (last patch) also has another issue so I need to fix
that too.

Best regards

Re: [PATCH 1/2] dbus: Better search for matching rules in the filter tree

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

Hi Andrew,

On 04/28/2016 10:31 PM, Andrew Zaborowski wrote:
> When adding new filter rules, perform a more complete search for
> existing rules that already cover the rule being added by removing the
> requierement that levels in the tree are ordered by the match type.
> This may add overhead to adding new rules in some situations but will
> reduce the number of server-side rules being used and the number of
> requests.  This covers a scenario where first a watch is added for:
>
> type=signal, path=/
>
> followed by
>
> type=signal, sender=org.test, path=/
>
> I believe there are some more tricky scenarios which are not covered
> specifically when more than one child of an existing node matches some
> condition of the rule being added.
> ---
>   ell/dbus-filter.c  | 67 +++++++++++++++++++++++++++++++++++++++++++-----------
>   ell/dbus-private.h |  8 +++----
>   2 files changed, 58 insertions(+), 17 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/2] unit: test a filter tree scenario with unordered paths

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

Hi Andrew,

On 04/28/2016 10:31 PM, Andrew Zaborowski wrote:
> Test a scenario in which some paths in the filter tree are not ordered
> by the match type.
> ---
>   unit/test-dbus-watch.c | 43 +++++++++++++++++++++++++++++++------------
>   1 file changed, 31 insertions(+), 12 deletions(-)
>

Applied, thanks.

Regards,
-Denis