Subject: Re: SFTP issues

Re: SFTP issues

From: Alexander Lamaison <swish_at_lammy.co.uk>
Date: Thu, 9 Feb 2012 12:06:13 +0000

On 9 February 2012 11:15, Peter Stuge <peter_at_stuge.se> wrote:
> Alexander Lamaison wrote:
>> The changes are more extensive that I'd planned so I'd appreciate some
>> careful review.  Basically, the total_read variable turned out to be
>> completely pointless.
>
> Yes. What about the loop? It's kept for the chunk stuff, so that
> libssh2_sftp_read() never returns 0 bytes to the caller unless EOF?

Yes. Originally I got rid of the loop but then realised we might read 0.

> I think it would be nice to return EAGAIN if we read 0 bytes from
> transport.

I like this idea. That would be cleaner and might remove the weird
situation where we return 0 at the end of the function without it
being EOF.

>> @@ -1348,13 +1334,13 @@ static ssize_t sftp_read(LIBSSH2_SFTP_HANDLE * handle, char *buffer,
>>                      filep->offset_sent -= (chunk->len - rc32);
>>                  }
>>
>> -                if(total_read + rc32 > buffer_size) {
>> +                if(rc32 > buffer_size) {
>>                      /* figure out the overlap amount */
>> -                    filep->data_left = (total_read + rc32) - buffer_size;
>> +                    filep->data_left = rc32 - buffer_size;
>
> Doesn't this lose the bytes that we received from transport but which
> do not fit into the buffer given to us by the user?

Not unless it did so before. total_read was always 0 at this point so
the changes shouldn't make any difference.

>> +                /* remove the chunk we just processed keeping track of the
>> +                 * next one in case we need it */
>> +                next = _libssh2_list_next(&chunk->node);
>> +                _libssh2_list_remove(&chunk->node);
>> +                LIBSSH2_FREE(session, chunk);
>> +                chunk = NULL;
>> +
>> +                if(rc32 > 0) {
>> +                    /* we must return as we wrote some data to the buffer */
>> +                    return rc32;
>> +                } else {
>> +                    /* A zero-byte read is not necessarily EOF so we must not
>> +                     * return 0 (that would signal EOF to the caller) so
>> +                     * instead we carry on to the next chunk */
>> +                    chunk = next;
>>                  }
>
> Here I suggest return EAGAIN instead of having the loop and chunk
> messyness.

I'll try this out tonight.

Alex

_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2012-02-09