From b215ec0af597a822667baf22aac5b234c206a89d Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Sat, 13 Nov 2010 23:13:21 +0100 Subject: [PATCH] _libssh2_channel_write: revert channel_write() use The attempts made to have _libssh2_channel_write() accept larger pieces of data and split up the data by itself into 32700 byte chunks and pass them on to channel_write() in a loop as a way to do faster operations on larger data blocks was a failed attempt. The reason why it is difficult: The API only allows EAGAIN or a length to be returned. When looping over multiple blocks to get sent, one block can get sent and the next might not. And yet: when transport_send() has returned EAGAIN we must not call it again with new data until it has returned OK on the existing data it is still working on. This makes it a mess and we do get a much easier job by simply returning the bytes or EAGAIN at once, as in the EAGAIN case we can assume that we will be called with the same arguments again and transport_send() will be happy. Unfortunately, I think we take a small performance hit by not being able to do this. --- src/channel.c | 122 +++++++++++---------------------------------- src/libssh2_priv.h | 4 -- 2 files changed, 29 insertions(+), 97 deletions(-) diff --git a/src/channel.c b/src/channel.c index 6112b04..ff1fb4d 100644 --- a/src/channel.c +++ b/src/channel.c @@ -1969,7 +1969,7 @@ _libssh2_channel_packet_data_len(LIBSSH2_CHANNEL * channel, int stream_id) } /* - * channel_write + * _libssh2_channel_write * * Send data to a channel. Note that if this returns EAGAIN, the caller must * call this function again with the SAME input arguments. @@ -1977,18 +1977,21 @@ _libssh2_channel_packet_data_len(LIBSSH2_CHANNEL * channel, int stream_id) * Returns: number of bytes sent, or if it returns a negative number, that is * the error code! */ -static ssize_t -channel_write(LIBSSH2_CHANNEL *channel, int stream_id, - const unsigned char *buf, size_t buflen) +ssize_t +_libssh2_channel_write(LIBSSH2_CHANNEL *channel, int stream_id, + const unsigned char *buf, size_t buflen) { + int rc = 0; LIBSSH2_SESSION *session = channel->session; - int rc; ssize_t wrote = 0; /* counter for this specific this call */ - /* In theory we could split larger buffers into several smaller packets, - * but for now we instead only deal with the first 32K in this call and - * assume the app will call it again with the rest! The 32K is a - * conservative limit based on the text in RFC4253 section 6.1. + /* In theory we could split larger buffers into several smaller packets + * but it turns out to be really hard and nasty to do while still offering + * the API/prototype. + * + * Instead we only deal with the first 32K in this call and for the parent + * function to call it again with the remainder! 32K is a conservative + * limit based on the text in RFC4253 section 6.1. */ if(buflen > 32700) buflen = 32700; @@ -1996,7 +1999,23 @@ channel_write(LIBSSH2_CHANNEL *channel, int stream_id, if (channel->write_state == libssh2_NB_state_idle) { unsigned char *s = channel->write_packet; - /* drain the incoming flow first */ + _libssh2_debug(channel->session, LIBSSH2_TRACE_CONN, + "Writing %d bytes on channel %lu/%lu, stream #%d", + (int) buflen, channel->local.id, channel->remote.id, + stream_id); + + if (channel->local.close) + return _libssh2_error(channel->session, + LIBSSH2_ERROR_CHANNEL_CLOSED, + "We've already closed this channel"); + else if (channel->local.eof) + return _libssh2_error(channel->session, + LIBSSH2_ERROR_CHANNEL_EOF_SENT, + "EOF has already been received, " + "data might be ignored"); + + /* drain the incoming flow first, mostly to make sure we get all + * pending window adjust packets */ do rc = _libssh2_transport_read(session); while (rc > 0); @@ -2080,89 +2099,6 @@ channel_write(LIBSSH2_CHANNEL *channel, int stream_id, return LIBSSH2_ERROR_INVAL; /* reaching this point is really bad */ } -/* - * _libssh2_channel_write - * - * Send data to a channel. Note that if this returns EAGAIN, the caller must - * call this function again with the SAME input arguments. - * - * Returns: number of bytes sent, or if it returns a negative number, that is - * the error code! - */ -ssize_t -_libssh2_channel_write(LIBSSH2_CHANNEL *channel, int stream_id, - const unsigned char *buf, size_t buflen) -{ - int rc = 0; - ssize_t wrote = 0; - - if(channel->transport_buf) { - /* previous EAGAIN situation, take care of this first */ - rc = channel_write(channel, - channel->transport_streamid, - channel->transport_buf, - channel->transport_buflen); - if(rc == LIBSSH2_ERROR_EAGAIN) - /* still EAGAIN, get out */ - return rc; - else if(rc < 0) - return rc; - - if(rc != (int)channel->transport_buflen) { - /* previous buffer not drained yet */ - channel->transport_buf += rc; - channel->transport_buflen -= rc; - return LIBSSH2_ERROR_EAGAIN; - } - - /* all is sent, clear the buf pointer */ - channel->transport_buf = NULL; - wrote += rc; - } - - _libssh2_debug(channel->session, LIBSSH2_TRACE_CONN, - "Writing %d bytes on channel %lu/%lu, stream #%d", - (int) buflen, channel->local.id, channel->remote.id, - stream_id); - - if (channel->local.close) { - return _libssh2_error(channel->session, LIBSSH2_ERROR_CHANNEL_CLOSED, - "We've already closed this channel"); - } - - if (channel->local.eof) { - return _libssh2_error(channel->session, LIBSSH2_ERROR_CHANNEL_EOF_SENT, - "EOF has already been received, " - "data might be ignored"); - } - - do { - rc = channel_write(channel, stream_id, buf, buflen); - - if(rc < 0) { - if(rc == LIBSSH2_ERROR_EAGAIN) { - /* store the buf/buflen pair and use that in the next call - again to flush the transport layer */ - channel->transport_streamid = stream_id; - channel->transport_buf = buf; - channel->transport_buflen = buflen; - if(wrote) - return wrote; - - /* nothing written, return EAGAIN */ - return rc; - } - return rc; - } - wrote += rc; - buf += rc; - buflen -= rc; - - } while(buflen); - - return wrote; -} - /* * libssh2_channel_write_ex * diff --git a/src/libssh2_priv.h b/src/libssh2_priv.h index 364ba05..d281c11 100644 --- a/src/libssh2_priv.h +++ b/src/libssh2_priv.h @@ -396,10 +396,6 @@ struct _LIBSSH2_CHANNEL size_t write_packet_len; size_t write_bufwrite; - int transport_streamid; - const unsigned char *transport_buf; - size_t transport_buflen; - /* State variables used in libssh2_channel_close() */ libssh2_nonblocking_states close_state; unsigned char close_packet[5];