From e0478a46c5c97a1aef87b74d8b0583b7dff1d2c1 Mon Sep 17 00:00:00 2001 From: Vincent Bernat Date: Thu, 30 Aug 2012 14:53:27 +0200 Subject: [PATCH] client: use libevent's bufferevent infrastructure to handle clients Previously, lldpd would block if the unix socket with the client was full (buggy client). This was not really a problem since a client needs to output some data to get some other data and therefore only evil clients may block lldpd. However, with (future) notifications, a hang client could stop lldpd from working. We may still eat a lot of memory if a client stay stuck. --- src/ctl.c | 107 +------------------------------------------- src/ctl.h | 13 +++++- src/daemon/client.c | 15 ++++--- src/daemon/event.c | 105 +++++++++++++++++++++++++++++++++---------- src/daemon/lldpd.h | 7 ++- 5 files changed, 108 insertions(+), 139 deletions(-) diff --git a/src/ctl.c b/src/ctl.c index c37f6d01..72db53bb 100644 --- a/src/ctl.c +++ b/src/ctl.c @@ -95,111 +95,6 @@ ctl_cleanup(char *name) LLOG_WARN("unable to unlink %s", name); } -/** Header for the control protocol. - * - * The protocol is pretty simple. We send a single message containing the - * provided message type with the message length, followed by the message - * content. - */ -struct hmsg_header { - enum hmsg_type type; - size_t len; -}; - -/** - * Send a message with the control protocol. - * - * @param fd The file descriptor that should be used. - * @param type The message type to be sent. - * @param t The buffer containing the message content. Can be @c NULL if the - * message is empty. - * @param len The length of the buffer containing the message content. - * @return The number of bytes written or -1 in case of error. - */ -int -ctl_msg_send(int fd, enum hmsg_type type, void *t, size_t len) -{ - struct iovec iov[2]; - struct hmsg_header hdr; - memset(&hdr, 0, sizeof(struct hmsg_header)); - hdr.type = type; - hdr.len = len; - iov[0].iov_base = &hdr; - iov[0].iov_len = sizeof(struct hmsg_header); - iov[1].iov_base = t; - iov[1].iov_len = len; - return writev(fd, iov, t?2:1); -} - -/** - * Receive a message with the control protocol. - * - * @param fd The file descriptor that should be used. - * @param type[out] The type of the received message. - * @param t The buffer containing the message content. - * @return The size of the returned buffer. 0 if the message is empty. -1 if - * there is an error. - */ -int -ctl_msg_recv(int fd, enum hmsg_type *type, void **t) -{ - int n, flags = -1; - struct hmsg_header hdr; - *type = NONE; *t = NULL; - /* First, we read the header to know the size of the message */ - if ((n = read(fd, &hdr, sizeof(struct hmsg_header))) == -1) { - LLOG_WARN("unable to read message header"); - return -1; - } - if (n == 0) - /* Remote closed the connection. */ - return -1; - if (n < sizeof(struct hmsg_header)) { - LLOG_WARNX("message received too short (%d)", n); - goto recv_error; - } - if (hdr.len > (1<<15)) { - LLOG_WARNX("message received is too large"); - goto recv_error; - } - if (hdr.len == 0) { - /* No answer */ - *type = hdr.type; - return 0; - } - /* Now, we read the remaining message. We need to use non-blocking stuff - * just in case the message was truncated. */ - if ((*t = malloc(hdr.len)) == NULL) { - LLOG_WARNX("not enough space available for incoming message"); - goto recv_error; - } - if ((flags = fcntl(fd, F_GETFL, 0)) == -1 || - fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1) { - LLOG_WARN("unable to set socket access mode to non blocking"); - goto recv_error; - } - if ((n = read(fd, *t, hdr.len)) == -1) { - if (errno == EAGAIN || errno == EWOULDBLOCK) { - LLOG_WARNX("message seems truncated"); - goto recv_error; - } - LLOG_WARN("unable to read incoming request"); - goto recv_error; - } - if (n != hdr.len) { - LLOG_WARNX("received message is too short (%d < %zu)", - n, hdr.len); - goto recv_error; - } - fcntl(fd, F_SETFL, flags); /* No error check */ - *type = hdr.type; - return hdr.len; -recv_error: - free(*t); *t = NULL; - if (flags != -1) fcntl(fd, F_SETFL, flags); - return -1; -} - /** * Serialize and "send" a structure through the control protocol. * @@ -214,6 +109,8 @@ recv_error: * @param t The structure to be serialized and sent. * @param mi The appropriate marshal structure for serialization. * @return -1 in case of failure, 0 in case of success. + * + * Make sure this function logic matches the server-side one: @c levent_ctl_recv(). */ int ctl_msg_send_unserialized(uint8_t **output_buffer, size_t *output_len, diff --git a/src/ctl.h b/src/ctl.h index 6f8b398e..d6e5e210 100644 --- a/src/ctl.h +++ b/src/ctl.h @@ -30,12 +30,21 @@ enum hmsg_type { SET_PORT, /* Set port-related information (location, power, policy) */ }; +/** Header for the control protocol. + * + * The protocol is pretty simple. We send a single message containing the + * provided message type with the message length, followed by the message + * content. + */ +struct hmsg_header { + enum hmsg_type type; + size_t len; +}; + /* ctl.c */ int ctl_create(char *); int ctl_connect(char *); void ctl_cleanup(char *); -int ctl_msg_send(int, enum hmsg_type, void *, size_t); -int ctl_msg_recv(int, enum hmsg_type *, void **); int ctl_msg_send_unserialized(uint8_t **, size_t *, enum hmsg_type, diff --git a/src/daemon/client.c b/src/daemon/client.c index 909b3801..8ad5c32c 100644 --- a/src/daemon/client.c +++ b/src/daemon/client.c @@ -209,22 +209,23 @@ static struct client_handle client_handles[] = { { 0, NULL } }; int -client_handle_client(struct lldpd *cfg, int fd, - enum hmsg_type type, void *buffer, int n) +client_handle_client(struct lldpd *cfg, + ssize_t(*send)(void *, int, void *, size_t), + void *out, + enum hmsg_type type, void *buffer, size_t n) { struct client_handle *ch; - void *answer; int len, sent; + void *answer; size_t len, sent; for (ch = client_handles; ch->handle != NULL; ch++) { if (ch->type == type) { answer = NULL; len = 0; - len = ch->handle(cfg, &type, buffer, n, &answer); - if ((sent = ctl_msg_send(fd, type, answer, len)) == -1) - LLOG_WARN("unable to send answer to client"); + len = ch->handle(cfg, &type, buffer, n, &answer); + sent = send(out, type, answer, len); free(answer); return sent; } } - + LLOG_WARNX("unknown message request (%d) received", type); return -1; diff --git a/src/daemon/event.c b/src/daemon/event.c index 9d0d8005..b557019a 100644 --- a/src/daemon/event.c +++ b/src/daemon/event.c @@ -19,7 +19,10 @@ #include #include +#include #include +#include +#include static void levent_log_cb(int severity, const char *msg) @@ -191,25 +194,82 @@ levent_snmp_update(struct lldpd *cfg) struct lldpd_one_client { struct lldpd *cfg; - struct event *ev; + struct bufferevent *bev; }; +static ssize_t +levent_ctl_send(void *out, int type, void *data, size_t len) +{ + struct lldpd_one_client *client = out; + struct bufferevent *bev = client->bev; + struct hmsg_header hdr = { .len = len, .type = type }; + bufferevent_disable(bev, EV_WRITE); + if (bufferevent_write(bev, &hdr, sizeof(struct hmsg_header)) == -1 || + (len > 0 && bufferevent_write(bev, data, len) == -1)) { + LLOG_WARNX("unable to create answer to client"); + bufferevent_free(bev); + free(client); + return -1; + } + bufferevent_enable(bev, EV_WRITE); + return len; +} + static void -levent_ctl_recv(evutil_socket_t fd, short what, void *arg) +levent_ctl_recv(struct bufferevent *bev, void *ptr) { - struct lldpd_one_client *client = arg; - enum hmsg_type type; - void *buffer = NULL; - int n; - (void)what; + struct lldpd_one_client *client = ptr; + struct evbuffer *buffer = bufferevent_get_input(bev); + size_t buffer_len = evbuffer_get_length(buffer); + struct hmsg_header hdr; + void *data = NULL; + + if (buffer_len < sizeof(struct hmsg_header)) + return; /* Not enough data yet */ + if (evbuffer_copyout(buffer, &hdr, + sizeof(struct hmsg_header)) != sizeof(struct hmsg_header)) { + LLOG_WARNX("not able to read header"); + return; + } + if (hdr.len > (1<<15)) { + LLOG_WARNX("message received is too large"); + goto recv_error; + } + + if (buffer_len < hdr.len + sizeof(struct hmsg_header)) + return; /* Not enough data yet */ + if (hdr.len > 0 && (data = malloc(hdr.len)) == NULL) { + LLOG_WARNX("not enough memory"); + goto recv_error; + } + evbuffer_drain(buffer, sizeof(struct hmsg_header)); + if (hdr.len > 0) evbuffer_remove(buffer, data, hdr.len); + if (client_handle_client(client->cfg, + levent_ctl_send, client, + hdr.type, data, hdr.len) == -1) goto recv_error; + free(data); + return; + +recv_error: + free(data); + bufferevent_free(bev); + free(client); +} - if ((n = ctl_msg_recv(fd, &type, &buffer)) == -1 || - client_handle_client(client->cfg, fd, type, buffer, n) == -1) { - close(fd); - event_free(client->ev); +static void +levent_ctl_event(struct bufferevent *bev, short events, void *ptr) +{ + struct lldpd_one_client *client = ptr; + if (events & BEV_EVENT_ERROR) { + LLOG_WARNX("an error occurred with client: %s", + evutil_socket_error_to_string(EVUTIL_SOCKET_ERROR())); + bufferevent_free(bev); + free(client); + } else if (events & BEV_EVENT_EOF) { + LLOG_DEBUG("client has been disconnected"); + bufferevent_free(bev); free(client); } - free(buffer); } static void @@ -231,22 +291,21 @@ levent_ctl_accept(evutil_socket_t fd, short what, void *arg) } client->cfg = cfg; evutil_make_socket_nonblocking(s); - if ((client->ev = event_new(cfg->g_base, s, - EV_READ | EV_PERSIST, - levent_ctl_recv, - client)) == NULL) { - LLOG_WARNX("unable to allocate a new event for new client"); - goto accept_failed; - } - if (event_add(client->ev, NULL) == -1) { - LLOG_WARNX("unable to schedule new event for new client"); + if ((client->bev = bufferevent_socket_new(cfg->g_base, s, + BEV_OPT_CLOSE_ON_FREE)) == NULL) { + LLOG_WARNX("unable to allocate a new buffer event for new client"); goto accept_failed; } + bufferevent_setcb(client->bev, + levent_ctl_recv, NULL, levent_ctl_event, + client); + bufferevent_enable(client->bev, EV_READ | EV_WRITE); return; accept_failed: - if (client && client->ev) event_free(client->ev); + if (client && client->bev) { + bufferevent_free(client->bev); + } else close(s); free(client); - close(s); } static void diff --git a/src/daemon/lldpd.h b/src/daemon/lldpd.h index 25135a52..9d942213 100644 --- a/src/daemon/lldpd.h +++ b/src/daemon/lldpd.h @@ -237,8 +237,11 @@ struct client_handle { void *, int, void **); }; -int client_handle_client(struct lldpd *, int, - enum hmsg_type, void *, int); +int +client_handle_client(struct lldpd *cfg, + ssize_t(*send)(void *, int, void *, size_t), + void *, + enum hmsg_type type, void *buffer, size_t n); /* priv.c */ void priv_init(char*, int, uid_t, gid_t); -- 2.39.5