Daniel Stenberg wrote: >> There is for SFTP, since it is a packet based protocol. Each FXP >> packet starts with a length, .. >> if lower layers can not reliably send the full packet. > > I know that's how SFTP works and I still don't understand how that > protocol design prevents our API/ABI to work.
Trace through the code then. Use sftp_write.c but increase buffer size to >=32768 and you will notice the problem with a lower layer processing only part of what is a packet in a higher layer. > The fact that send() and recv() return such values, and all SSL > libraries I've used return such numbers for their > SSL_send()/SSL_recv() equivalents tell me that's a well used > paradigm that I think we should keep. Sure, if it works. But you should also be prepared to change it if that makes sense. SSL is a stream but SSH transport and SFTP aren't. There's no problem for SSH channels, since they are also streams. >> The current API without sync between upper and lower layer numbers >> is bad since information gets lost. > > No, it's not lost information. Yes, libssh2 would not send incomplete SFTP packets otherwise. >> In order to silently work, *each* layer would require *another* state >> machine which would be used to split an upper layer buffer into >> multiple manageable chunks - which trickles down through all lower >> layers whenever an application happens to hit the buffer size limit >> for a particular layer. > > I disagree. Your description is one approach to rework this to function > optimally. It would not be optimal at all. If each layer has the same buffer size limit then there will be a split in every layer below the first one where the upper layer buffer exceeds the limit. This is what I meant by "trickles down". Each layer could coalesce splits from the higher layer, so that 32778 would get split first into 32768+10 and in the next layer where 10 more bytes are prepended, it would be split again into 32768+10+10, but then that could be coalesced into 32768+20. IMO it would be bad to have that kind of garbage collector in libssh2. > The current way of doing it still works. It's just a bit lame with > buffer sizes. Define lame? Play with the sftp buffer size and you'll notice the problem. It doesn't really work. >> I'd like to work towards less state within libssh2, not more. :) > > We will always have lots of state within libssh2. Things could be changed around to keep less state in libssh2 without losing any functionality, e.g. by having applications prepare packets before they send them. And if you trusted select() for blocking fds then the required state could be reduced even further. > The challenge is to make the states readable and understandable. One way is to expose them to application writers by separating _prepare from _send. That might also make it easier to improve internal algorithms in the library, like the memory allocation stuff. >> The ABI could remain, but the API semantics need to change, and it's not >> so nice to make an incompatible API change without also changing the ABI. >> (soname I guess, but is there a big difference?) > > If API semantics change, then I claim the ABI is changed as well. ABI is > not just about being able to still link IMHO. At least we agree on this. :) >> there's no way for the top layer to know what is actually going >> out on the wire - yet "bytes sent" is being used to measure >> progress and track state. > > Sent bytes can be used to measure state within the protocol layer, > but it can of course not tell anything about the state in other > layers. And it _can_ be used to measure progress, again in exactly > the same way we can track a TCP transfer by counting how many bytes > send() tells us it has transfered. There's a big difference in that TCP is a stream, so it's fine for the stack to send fewer bytes than requested. SSH is a packet transport and complete packets must always be sent. The simplest way out for the library is to give applications access to the real number of bytes that need to be sent by the lowest layer. That is not to say it is neccessarily the best way. But it would be quite transparent, and make things very easy for anyone who is used to read()/write() semantics. > I agree that we have some internal confusions about a return value > 0, but the fact is that we MUST never treat reading or writing zero > bytes as an error exactly because of the reasons you mention: it > simply means that we didn't read or send any payload data but lots > of protocol bits may still have flown over the wire. 0 is progress, > just very little progress ;-) > > Any other dealing with 0 is a bug in my eyes. It only works when all lower layer extra bytes come before upper layer bytes. This isn't the case, I think I mentioned transport layer padding and MAC which both are appendeed after the upper layer bytes. There may be more instances of this. The library then has to lie either about bytes in the beginning or in the end, I don't think that's very pretty either way. >> Another solution is that upper layer learns about lower layer needs; >> instead of counting up to 32768 in the app, it gets the 32820 number to >> measure progress on the wire. This is mildly confusing for app writers, >> but it would work well. > > It would be completely crazy in my mind. What exactly would be > included in that count? The real number of bytes that need to go out on the TCP socket. It also gives the application feedback about the total protocol overhead for the chosen buffer size, which could be used by smart applications to adjust internal buffers. (Maybe together with remote window information, and some timings made by the app.) > The SFTP_write() function is for sending N bytes to the peer. > We want to know how many of those N bytes that were sent - and thus > how many more we need to send before we are done. That's not the full story; applications need to know how many of the N+overhead bytes that have been sent, in order to know when the SFTP packet with N data bytes has been sent, and the next packet can be started. >> A better solution would be great! In any case I don't think the >> API can remain as is. > > I disagree. The problems are not in the API or ABI, the problems > are in the implementation of the functions we have. Can you go into detail about how you think that the implementations could be improved? //Peter _______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
