From 16449c4f4972ffad500951db5c71403cae0422e7 Mon Sep 17 00:00:00 2001 From: Guillaume Nault Date: Wed, 27 Apr 2016 21:02:37 +0200 Subject: cli: fix partial line duplication and truncation When queueing output data for later write(), the 'n' first bytes of the buffer have already been sent (we have n > 0 if EAGAIN was returned after some other write() calls succeeded). Therefore, we need to skip these bytes when initialising the buffer to be queued. The size passed to memcpy() did already take that space into account. Signed-off-by: Guillaume Nault --- accel-pppd/cli/tcp.c | 2 +- accel-pppd/cli/telnet.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/accel-pppd/cli/tcp.c b/accel-pppd/cli/tcp.c index 2a6bfdee..9061edd1 100644 --- a/accel-pppd/cli/tcp.c +++ b/accel-pppd/cli/tcp.c @@ -109,7 +109,7 @@ static int cli_client_send(struct cli_client_t *tcln, const void *_buf, int size if (errno == EAGAIN) { b = _malloc(sizeof(*b) + size - n); b->size = size - n; - memcpy(b->buf, buf, size - n); + memcpy(b->buf, buf + n, size - n); queue_buffer(cln, b); triton_md_enable_handler(&cln->hnd, MD_MODE_WRITE); diff --git a/accel-pppd/cli/telnet.c b/accel-pppd/cli/telnet.c index de2f39ff..b8ca8231 100644 --- a/accel-pppd/cli/telnet.c +++ b/accel-pppd/cli/telnet.c @@ -157,7 +157,7 @@ static int telnet_send(struct telnet_client_t *cln, const void *_buf, int size) if (errno == EAGAIN) { b = _malloc(sizeof(*b) + size - n); b->size = size - n; - memcpy(b->buf, buf, size - n); + memcpy(b->buf, buf + n, size - n); queue_buffer(cln, b); triton_md_enable_handler(&cln->hnd, MD_MODE_WRITE); -- cgit v1.2.3 From 72b21d0537b49f23254bdf63ad9a3d1a61b3bbbe Mon Sep 17 00:00:00 2001 From: Guillaume Nault Date: Wed, 27 Apr 2016 21:02:39 +0200 Subject: cli: fix data output miss-ordering In tcp and telnet backends, the first buffer been queued is directly pointed to by cln->xmit_buf. It's not added to cln->xmit_queue. Therefore testing if ->xmit_queue is empty doesn't reliably tells if data has already been queued. We should test if ->xmit_buf is non-NULL instead. This is reliable because ->xmit_buf is re-filled with the first buffer from ->xmit_queue after every successful write(). Failure to properly check if data has already been queued can lead to message miss-ordering because cli_client_send() or telnet_send() will try to directly write() their input buffer, effectively bypassing the one previously queued up in ->xmit_buf. Signed-off-by: Guillaume Nault --- accel-pppd/cli/tcp.c | 2 +- accel-pppd/cli/telnet.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/accel-pppd/cli/tcp.c b/accel-pppd/cli/tcp.c index 9061edd1..3a00cfec 100644 --- a/accel-pppd/cli/tcp.c +++ b/accel-pppd/cli/tcp.c @@ -95,7 +95,7 @@ static int cli_client_send(struct cli_client_t *tcln, const void *_buf, int size if (cln->disconnect) return -1; - if (!list_empty(&cln->xmit_queue)) { + if (cln->xmit_buf) { b = _malloc(sizeof(*b) + size); b->size = size; memcpy(b->buf, buf, size); diff --git a/accel-pppd/cli/telnet.c b/accel-pppd/cli/telnet.c index b8ca8231..6c41769e 100644 --- a/accel-pppd/cli/telnet.c +++ b/accel-pppd/cli/telnet.c @@ -143,7 +143,7 @@ static int telnet_send(struct telnet_client_t *cln, const void *_buf, int size) if (cln->disconnect) return -1; - if (!list_empty(&cln->xmit_queue)) { + if (cln->xmit_buf) { b = _malloc(sizeof(*b) + size); b->size = size; memcpy(b->buf, buf, size); -- cgit v1.2.3 From 20e1eff0c9afeab494a1c24265a4a2d28058d9dd Mon Sep 17 00:00:00 2001 From: Guillaume Nault Date: Wed, 27 Apr 2016 21:02:42 +0200 Subject: cli: flush pending data before disconnecting The telnet and tcp servers disconnect as soon as they receive the 'exit' command or see a disconnection from the client. In this case, all data queued for transmission are lost. This can lead to truncated output when big amount of data is being sent. For example, on a moderately loaded server with a few thouthands connections, the output of the 'accel-cmd show sessions' command can be truncated. The problem is that accel-cmd sends the 'show sessions' command, followed by 'exit'. It does so because it has to stop running once all data has been received from the server. But it never knows whether more data are going to arrive. Disconnection must then come from the server, hence the use of 'exit' (although the same effect could be achieved with shutdown(SHUT_WR)). The telnet and tcp modules behave very similarly and are modified in the same way: * For a soft disconnection, cln_read() doesn't call disconnect() anymore if there are data queued for transmission. Instead it sets the 'disconnect' flag and stops listening to its peer (no need to process further messages). * cln_write() checks the 'disconnect' flag once it has sent all pending data and actually performs the disconnection if necessary. Signed-off-by: Guillaume Nault --- accel-pppd/cli/tcp.c | 45 ++++++++++++++++++++++++++++--------------- accel-pppd/cli/telnet.c | 51 ++++++++++++++++++++++++++++++++----------------- 2 files changed, 63 insertions(+), 33 deletions(-) diff --git a/accel-pppd/cli/tcp.c b/accel-pppd/cli/tcp.c index 3a00cfec..051ff84c 100644 --- a/accel-pppd/cli/tcp.c +++ b/accel-pppd/cli/tcp.c @@ -147,7 +147,7 @@ static int cln_read(struct triton_md_handler_t *h) while (1) { n = read(h->fd, cln->cmdline + cln->recv_pos, RECV_BUF_SIZE - 1 - cln->recv_pos); if (n == 0) - break; + goto disconn_soft; if (n < 0) { if (errno != EAGAIN) log_error("cli: read: %s\n", strerror(errno)); @@ -162,7 +162,7 @@ static int cln_read(struct triton_md_handler_t *h) if (!d) { if (cln->recv_pos == RECV_BUF_SIZE - 1) { log_warn("cli: tcp: recv buffer overflow\n"); - goto drop; + goto disconn_hard; } break; } @@ -171,7 +171,7 @@ static int cln_read(struct triton_md_handler_t *h) if (!cln->auth) { if (strcmp((char *)cln->cmdline, conf_cli_passwd)) - goto drop; + goto disconn_hard; cln->auth = 1; } else { if (conf_verbose == 2) @@ -181,15 +181,25 @@ static int cln_read(struct triton_md_handler_t *h) } if (cln->disconnect) - goto drop; + goto disconn_soft; cln->recv_pos -= (uint8_t *)d + 1 - cln->cmdline; memmove(cln->cmdline, d + 1, cln->recv_pos); } } -drop: +disconn_soft: + /* Wait for pending data to be transmitted before disconnecting */ + if (cln->xmit_buf) { + triton_md_disable_handler(&cln->hnd, MD_MODE_READ); + cln->disconnect = 1; + + return 0; + } + +disconn_hard: disconnect(cln); + return -1; } @@ -198,10 +208,7 @@ static int cln_write(struct triton_md_handler_t *h) struct tcp_client_t *cln = container_of(h, typeof(*cln), hnd); int k; - if (!cln->xmit_buf) - return 0; - - while (1) { + while (cln->xmit_buf) { for (; cln->xmit_pos < cln->xmit_buf->size; cln->xmit_pos += k) { k = write(cln->hnd.fd, cln->xmit_buf->buf + cln->xmit_pos, cln->xmit_buf->size - cln->xmit_pos); if (k < 0) { @@ -209,8 +216,7 @@ static int cln_write(struct triton_md_handler_t *h) return 0; if (errno != EPIPE) log_error("cli: tcp: write: %s\n", strerror(errno)); - disconnect(cln); - return -1; + goto disconn; } } @@ -219,16 +225,25 @@ static int cln_write(struct triton_md_handler_t *h) if (list_empty(&cln->xmit_queue)) { cln->xmit_buf = NULL; - break; + } else { + cln->xmit_buf = list_first_entry(&cln->xmit_queue, + typeof(*cln->xmit_buf), + entry); + list_del(&cln->xmit_buf->entry); } - - cln->xmit_buf = list_entry(cln->xmit_queue.next, typeof(*cln->xmit_buf), entry); - list_del(&cln->xmit_buf->entry); } + if (cln->disconnect) + goto disconn; + triton_md_disable_handler(&cln->hnd, MD_MODE_WRITE); return 0; + +disconn: + disconnect(cln); + + return -1; } static int serv_read(struct triton_md_handler_t *h) diff --git a/accel-pppd/cli/telnet.c b/accel-pppd/cli/telnet.c index 6c41769e..9ef2ea84 100644 --- a/accel-pppd/cli/telnet.c +++ b/accel-pppd/cli/telnet.c @@ -477,10 +477,8 @@ static int cln_read(struct triton_md_handler_t *h) while (1) { n = read(h->fd, recv_buf, RECV_BUF_SIZE); - if (n == 0) { - disconnect(cln); - return -1; - } + if (n == 0) + goto disconn_soft; if (n < 0) { if (errno != EAGAIN) log_error("cli: telnet: read: %s\n", strerror(errno)); @@ -492,13 +490,25 @@ static int cln_read(struct triton_md_handler_t *h) if (telnet_input_char(cln, recv_buf[i])) break; } - if (cln->disconnect) { - disconnect(cln); - return -1; - } + + if (cln->disconnect) + goto disconn_soft; } return 0; + +disconn_soft: + /* Wait for pending data to be transmitted before disconnecting */ + if (cln->xmit_buf) { + triton_md_disable_handler(&cln->hnd, MD_MODE_READ); + cln->disconnect = 1; + + return 0; + } + + disconnect(cln); + + return -1; } static int cln_write(struct triton_md_handler_t *h) @@ -506,10 +516,7 @@ static int cln_write(struct triton_md_handler_t *h) struct telnet_client_t *cln = container_of(h, typeof(*cln), hnd); int k; - if (!cln->xmit_buf) - return 0; - - while (1) { + while (cln->xmit_buf) { for (; cln->xmit_pos < cln->xmit_buf->size; cln->xmit_pos += k) { k = write(cln->hnd.fd, cln->xmit_buf->buf + cln->xmit_pos, cln->xmit_buf->size - cln->xmit_pos); if (k < 0) { @@ -517,8 +524,7 @@ static int cln_write(struct triton_md_handler_t *h) return 0; if (errno != EPIPE) log_error("cli: telnet: write: %s\n", strerror(errno)); - disconnect(cln); - return -1; + goto disconn; } } @@ -527,16 +533,25 @@ static int cln_write(struct triton_md_handler_t *h) if (list_empty(&cln->xmit_queue)) { cln->xmit_buf = NULL; - break; + } else { + cln->xmit_buf = list_first_entry(&cln->xmit_queue, + typeof(*cln->xmit_buf), + entry); + list_del(&cln->xmit_buf->entry); } - - cln->xmit_buf = list_entry(cln->xmit_queue.next, typeof(*cln->xmit_buf), entry); - list_del(&cln->xmit_buf->entry); } + if (cln->disconnect) + goto disconn; + triton_md_disable_handler(&cln->hnd, MD_MODE_WRITE); return 0; + +disconn: + disconnect(cln); + + return -1; } static int serv_read(struct triton_md_handler_t *h) -- cgit v1.2.3