* [PATCH 2/2] dbus: Create dbus->filter and name_cache lazily
2016-05-11 9:36 [PATCH 1/2] dbus: Add name_cache argument NULL checks Andrew Zaborowski
@ 2016-05-11 9:36 ` Andrew Zaborowski
2016-05-11 15:10 ` Denis Kenzior
2016-05-11 9:36 ` [PATCH 1/2] dbus: Utility to look up UNIX_FDS field in raw header Andrew Zaborowski
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Andrew Zaborowski @ 2016-05-11 9:36 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 6168 bytes --]
---
ell/dbus.c | 96 +++++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 63 insertions(+), 33 deletions(-)
diff --git a/ell/dbus.c b/ell/dbus.c
index 7cb4740..69222e9 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -96,6 +96,7 @@ struct l_dbus {
struct _dbus_object_tree *tree;
struct _dbus_name_cache *name_cache;
struct _dbus_filter *filter;
+ bool name_notify_enabled;
const struct l_dbus_ops *driver;
};
@@ -545,11 +546,6 @@ static void dbus_init(struct l_dbus *dbus, int fd)
dbus->signal_list = l_hashmap_new();
dbus->tree = _dbus_object_tree_new();
-
- dbus->name_cache = _dbus_name_cache_new(dbus, &dbus->driver->name_ops);
-
- dbus->filter = _dbus_filter_new(dbus, &dbus->driver->filter_ops,
- dbus->name_cache);
}
static void classic_free(struct l_dbus *dbus)
@@ -742,6 +738,18 @@ static bool classic_remove_match(struct l_dbus *dbus, unsigned int id)
return true;
}
+static void name_owner_changed_cb(struct l_dbus_message *message,
+ void *user_data)
+{
+ struct l_dbus *dbus = user_data;
+ char *name, *old, *new;
+
+ if (!l_dbus_message_get_arguments(message, "sss", &name, &old, &new))
+ return;
+
+ _dbus_name_cache_notify(dbus->name_cache, name, new);
+}
+
struct get_name_owner_request {
struct l_dbus_message *message;
struct l_dbus *dbus;
@@ -789,6 +797,26 @@ static bool classic_get_name_owner(struct l_dbus *bus, const char *name)
send_message(bus, false, req->message, get_name_owner_reply_cb,
req, l_free);
+ if (!bus->name_notify_enabled) {
+ static struct _dbus_filter_condition rule[] = {
+ { L_DBUS_MATCH_TYPE, "signal" },
+ { L_DBUS_MATCH_SENDER, DBUS_SERVICE_DBUS },
+ { L_DBUS_MATCH_PATH, DBUS_PATH_DBUS },
+ { L_DBUS_MATCH_INTERFACE, L_DBUS_INTERFACE_DBUS },
+ { L_DBUS_MATCH_MEMBER, "NameOwnerChanged" },
+ };
+
+ if (!bus->filter)
+ bus->filter = _dbus_filter_new(bus,
+ &bus->driver->filter_ops,
+ bus->name_cache);
+
+ _dbus_filter_add_rule(bus->filter, rule, L_ARRAY_SIZE(rule),
+ name_owner_changed_cb, bus);
+
+ bus->name_notify_enabled = true;
+ }
+
return true;
}
@@ -887,18 +915,6 @@ static const struct l_dbus_ops classic_ops = {
.name_acquire = classic_name_acquire,
};
-static void name_owner_changed_cb(struct l_dbus_message *message,
- void *user_data)
-{
- struct l_dbus *dbus = user_data;
- char *name, *old, *new;
-
- if (!l_dbus_message_get_arguments(message, "sss", &name, &old, &new))
- return;
-
- _dbus_name_cache_notify(dbus->name_cache, name, new);
-}
-
static struct l_dbus *setup_dbus1(int fd, const char *guid)
{
static const unsigned char creds = 0x00;
@@ -908,13 +924,6 @@ static struct l_dbus *setup_dbus1(int fd, const char *guid)
ssize_t written;
unsigned int i;
long flags;
- static struct _dbus_filter_condition rule[] = {
- { L_DBUS_MATCH_TYPE, "signal" },
- { L_DBUS_MATCH_SENDER, DBUS_SERVICE_DBUS },
- { L_DBUS_MATCH_PATH, DBUS_PATH_DBUS },
- { L_DBUS_MATCH_INTERFACE, L_DBUS_INTERFACE_DBUS },
- { L_DBUS_MATCH_MEMBER, "NameOwnerChanged" },
- };
if (snprintf(uid, sizeof(uid), "%d", geteuid()) < 1) {
close(fd);
@@ -963,9 +972,6 @@ static struct l_dbus *setup_dbus1(int fd, const char *guid)
l_io_set_read_handler(dbus->io, auth_read_handler, dbus, NULL);
l_io_set_write_handler(dbus->io, auth_write_handler, dbus, NULL);
- _dbus_filter_add_rule(dbus->filter, rule, L_ARRAY_SIZE(rule),
- name_owner_changed_cb, dbus);
-
return dbus;
}
@@ -1167,6 +1173,7 @@ static bool kdbus_get_name_owner(struct l_dbus *dbus, const char *name)
int fd = l_io_get_fd(dbus->io);
uint64_t owner_id;
char owner[32];
+ int r;
owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool, name);
@@ -1177,6 +1184,15 @@ static bool kdbus_get_name_owner(struct l_dbus *dbus, const char *name)
_dbus_name_cache_notify(dbus->name_cache, name, owner);
+ if (!dbus->name_notify_enabled) {
+ r = _dbus_kernel_enable_name_owner_notify(fd);
+ if (r < 0)
+ l_util_debug(dbus->debug_handler,
+ dbus->debug_data, strerror(-r));
+
+ dbus->name_notify_enabled = true;
+ }
+
return true;
}
@@ -1225,7 +1241,6 @@ static struct l_dbus *setup_kdbus(int fd)
{
struct l_dbus *dbus;
struct l_dbus_kdbus *kdbus;
- int r;
kdbus = l_new(struct l_dbus_kdbus, 1);
dbus = &kdbus->super;
@@ -1246,11 +1261,6 @@ static struct l_dbus *setup_kdbus(int fd)
l_idle_oneshot(kdbus_ready, dbus, NULL);
- r = _dbus_kernel_enable_name_owner_notify(fd);
- if (r < 0)
- l_util_debug(dbus->debug_handler,
- dbus->debug_data, strerror(-r));
-
return dbus;
}
@@ -1760,6 +1770,10 @@ LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
if (!name)
return 0;
+ if (!dbus->name_cache)
+ dbus->name_cache = _dbus_name_cache_new(dbus,
+ &dbus->driver->name_ops);
+
return _dbus_name_cache_add_watch(dbus->name_cache, name, connect_func,
disconnect_func, user_data,
destroy);
@@ -1767,6 +1781,9 @@ LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int id)
{
+ if (!dbus->name_cache)
+ return false;
+
return _dbus_name_cache_remove_watch(dbus->name_cache, id);
}
@@ -1876,6 +1893,16 @@ LIB_EXPORT unsigned int l_dbus_add_signal_watch(struct l_dbus *dbus,
va_end(args);
+ if (!dbus->filter) {
+ if (!dbus->name_cache)
+ dbus->name_cache = _dbus_name_cache_new(dbus,
+ &dbus->driver->name_ops);
+
+ dbus->filter = _dbus_filter_new(dbus,
+ &dbus->driver->filter_ops,
+ dbus->name_cache);
+ }
+
id = _dbus_filter_add_rule(dbus->filter, rule, rule_len,
signal_func, user_data);
@@ -1886,6 +1913,9 @@ LIB_EXPORT unsigned int l_dbus_add_signal_watch(struct l_dbus *dbus,
LIB_EXPORT bool l_dbus_remove_signal_watch(struct l_dbus *dbus, unsigned int id)
{
+ if (!dbus->filter)
+ return false;
+
return _dbus_filter_remove_rule(dbus->filter, id);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] dbus: Use blocking socket for Dbus1, adapt IO calls
2016-05-11 9:36 [PATCH 1/2] dbus: Add name_cache argument NULL checks Andrew Zaborowski
2016-05-11 9:36 ` [PATCH 2/2] dbus: Create dbus->filter and name_cache lazily Andrew Zaborowski
2016-05-11 9:36 ` [PATCH 1/2] dbus: Utility to look up UNIX_FDS field in raw header Andrew Zaborowski
@ 2016-05-11 9:36 ` Andrew Zaborowski
2016-05-11 16:07 ` Denis Kenzior
2016-05-11 15:12 ` [PATCH 1/2] dbus: Add name_cache argument NULL checks Denis Kenzior
3 siblings, 1 reply; 8+ messages in thread
From: Andrew Zaborowski @ 2016-05-11 9:36 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 8831 bytes --]
We need to handle partial reads and writes and FD passing with multiple
messages in the socket's queue so we may need multiple calls to send or
recv. With a blocking socket we can call them in the loop and simplify
buffering of message data.
---
ell/dbus.c | 220 +++++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 157 insertions(+), 63 deletions(-)
diff --git a/ell/dbus.c b/ell/dbus.c
index 69222e9..f6ae531 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -114,6 +114,8 @@ struct l_dbus_classic {
void *auth_command;
enum auth_state auth_state;
struct l_hashmap *match_strings;
+ int *fd_buf;
+ unsigned int num_fds;
};
struct message_callback {
@@ -397,6 +399,12 @@ static bool auth_write_handler(struct l_io *io, void *user_data)
l_util_hexdump(false, classic->auth_command, written,
dbus->debug_handler, dbus->debug_data);
+ if (written < len) {
+ memmove(classic->auth_command, classic->auth_command + written,
+ len + 1 - written);
+ return true;
+ }
+
l_free(classic->auth_command);
classic->auth_command = NULL;
@@ -436,9 +444,13 @@ static bool auth_read_handler(struct l_io *io, void *user_data)
offset = 0;
while (1) {
- len = recv(fd, ptr + offset, sizeof(buffer) - offset, 0);
- if (len < 1)
+ len = recv(fd, ptr + offset, sizeof(buffer) - offset,
+ MSG_DONTWAIT);
+ if (len < 1) {
+ if (len == -1 && errno == EINTR)
+ continue;
break;
+ }
offset += len;
}
@@ -552,6 +564,11 @@ static void classic_free(struct l_dbus *dbus)
{
struct l_dbus_classic *classic =
container_of(dbus, struct l_dbus_classic, super);
+ unsigned int i;
+
+ for (i = 0; i < classic->num_fds; i++)
+ close(classic->fd_buf[i]);
+ l_free(classic->fd_buf);
l_free(classic->auth_command);
l_hashmap_destroy(classic->match_strings, l_free);
@@ -563,48 +580,79 @@ static bool classic_send_message(struct l_dbus *dbus,
{
int fd = l_io_get_fd(dbus->io);
struct msghdr msg;
- struct iovec iov[2];
- ssize_t len;
+ struct iovec iov[2], *iovpos;
+ ssize_t r;
int *fds;
uint32_t num_fds = 0;
struct cmsghdr *cmsg;
+ int iovlen;
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));
+ iovpos = iov;
+ iovlen = 2;
- 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));
- }
+ while (1) {
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = iovpos;
+ msg.msg_iovlen = iovlen;
+
+ 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;
+ r = sendmsg(fd, &msg, 0);
+ if (r <= 0) {
+ if (r < 0 && errno != EINTR)
+ return false;
+
+ continue;
+ }
+
+ while ((size_t) r >= iovpos->iov_len) {
+ r -= iovpos->iov_len;
+ iovpos++;
+ iovlen--;
+
+ if (!iovlen)
+ break;
+ }
+
+ if (!iovlen)
+ break;
+
+ iovpos->iov_base += r;
+ iovpos->iov_len -= r;
+
+ /* The FDs have been transmitted, don't retransmit */
+ num_fds = 0;
+ }
return true;
}
static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
{
+ struct l_dbus_classic *classic =
+ container_of(dbus, struct l_dbus_classic, super);
int fd = l_io_get_fd(dbus->io);
struct dbus_header hdr;
struct msghdr msg;
- struct iovec iov[2];
+ struct iovec iov[2], *iovpos;
struct cmsghdr *cmsg;
- ssize_t len;
+ ssize_t len, r;
void *header, *body;
size_t header_size, body_size;
union {
@@ -613,6 +661,9 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
} fd_buf;
int *fds = NULL;
uint32_t num_fds = 0;
+ int iovlen;
+ struct l_dbus_message *message;
+ unsigned int i;
len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
if (len != DBUS_HEADER_SIZE)
@@ -629,38 +680,68 @@ 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);
+ iovpos = iov;
+ iovlen = 2;
- len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
- if (len < 0)
- goto cmsg_fail;
+ while (1) {
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = iovpos;
+ msg.msg_iovlen = iovlen;
+ msg.msg_control = &fd_buf;
+ msg.msg_controllen = sizeof(fd_buf);
- for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
- cmsg = CMSG_NXTHDR(&msg, cmsg)) {
- unsigned int i;
+ r = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC | MSG_WAITALL);
+ if (r < 0) {
+ if (errno != EINTR)
+ goto cmsg_fail;
- if (cmsg->cmsg_level != SOL_SOCKET ||
- cmsg->cmsg_type != SCM_RIGHTS)
continue;
+ }
+
+ for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
+ cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+ 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);
+ 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;
+ /* 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;
+ flags = fcntl(fds[i], F_GETFD, NULL);
+ if (flags < 0)
+ continue;
+
+ if (!(flags & FD_CLOEXEC))
+ fcntl(fds[i], F_SETFD,
+ flags | FD_CLOEXEC);
+ }
+
+ classic->fd_buf = l_realloc(classic->fd_buf,
+ (classic->num_fds + num_fds) *
+ sizeof(int));
+ memcpy(classic->fd_buf + classic->num_fds, fds,
+ num_fds * sizeof(int));
+ classic->num_fds += num_fds;
+ }
- if (!(flags & FD_CLOEXEC))
- fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
- }
+ while ((size_t) r >= iovpos->iov_len) {
+ r -= iovpos->iov_len;
+ iovpos++;
+ iovlen--;
+
+ if (!iovlen)
+ break;
+ }
+
+ if (!iovlen)
+ break;
+
+ iovpos->iov_base += r;
+ iovpos->iov_len -= r;
}
if (hdr.endian != DBUS_NATIVE_ENDIAN) {
@@ -675,11 +756,39 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
goto bad_msg;
}
- return dbus_message_build(header, header_size, body, body_size,
- fds, num_fds);
+ num_fds = _dbus_message_unix_fds_from_header(header, header_size);
+ if (num_fds > classic->num_fds)
+ goto bad_msg;
+
+ message = dbus_message_build(header, header_size, body, body_size,
+ classic->fd_buf, num_fds);
+
+ if (message && num_fds) {
+ if (classic->num_fds > num_fds) {
+ memmove(classic->fd_buf, classic->fd_buf + num_fds,
+ (classic->num_fds - num_fds) * sizeof(int));
+ classic->num_fds -= num_fds;
+ } else {
+ l_free(classic->fd_buf);
+
+ classic->fd_buf = NULL;
+ classic->num_fds = 0;
+ }
+ }
+
+ if (message)
+ return message;
bad_msg:
cmsg_fail:
+ for (i = 0; i < classic->num_fds; i++)
+ close(classic->fd_buf[i]);
+
+ l_free(classic->fd_buf);
+
+ classic->fd_buf = NULL;
+ classic->num_fds = 0;
+
l_free(header);
l_free(body);
@@ -923,7 +1032,6 @@ static struct l_dbus *setup_dbus1(int fd, const char *guid)
struct l_dbus_classic *classic;
ssize_t written;
unsigned int i;
- long flags;
if (snprintf(uid, sizeof(uid), "%d", geteuid()) < 1) {
close(fd);
@@ -940,20 +1048,6 @@ static struct l_dbus *setup_dbus1(int fd, const char *guid)
return NULL;
}
- flags = fcntl(fd, F_GETFL, NULL);
- if (flags < 0) {
- close(fd);
- return NULL;
- }
-
- /* Input handling requires non-blocking socket */
- if (!(flags & O_NONBLOCK)) {
- if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) < 0) {
- close(fd);
- return NULL;
- }
- }
-
classic = l_new(struct l_dbus_classic, 1);
dbus = &classic->super;
dbus->driver = &classic_ops;
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread