Subject: Re: Callback for channel data ready

Re: Callback for channel data ready

From: Mitchell Hashimoto <mitchell.hashimoto_at_gmail.com>
Date: Fri, 2 Mar 2012 10:39:32 -0800

On Fri, Mar 2, 2012 at 3:21 AM, Tom Weber <tom.weber_at_cryptzone.com> wrote:

>
> Hi!
>
> These are my thoughts...
>
> > On Wed, Feb 29, 2012 at 7:42 PM, Peter Stuge <peter_at_stuge.se> wrote:
> >> > > Sorry not at this time. I think we want callbacks though, so feel
> >> > > free to discuss and send patches. :)
>
> Callbacks sound nice in theory, but I've had my share of them, wrestling
> with
> PuTTY and Symbian code, and particularly those two together, which have
> mostly
> incompatible callback schemes.
>
> The problem is that you as an application developer don't control the
> context
> in which the callback is called.
>
> 1) You might want to disconnect and free the session, but you can't do that
> since you have to return to a libssh2_session_ routine, which expects the
> session to be valid.
>
> 2) You might want to process the event later, and then you need an
> additional
> mechanism to save the event.
>
> 3) During which function call, and from which thread, can we count on the
> callback being called? We must know that exactly, to handle things like
> mutexes and resource ownership correctly. (Preferably the callback pointer
> should be provided at the function call and not saved, to make this more
> clear.)
>
> By the way, signals suffer from the same problems...
>

Agreed, I'd like to stay away from callbacks now. It complicates many
details.

>
> >> There is no such thing.. libssh2 has evolved, and must continue to
> >> evolve. The libssh2 API can and should be expanded to become more
> >> useful, and the definition of useful should basically come from it's
> >> users.
>
> Agreed! :-)
>
> On Fri, March 2, 2012 03:26, Mitchell Hashimoto wrote:
> > Instead, I believe it would be nice to do something like the following:
> >
> > int libssh2_session_ready_channels(LIBSSH2_SESSION *, LIBSSH2_CHANNEL **)
> >
> > Where this would return a LIBSSH2_ERROR_EAGAIN if the socket would block
> > when nonblocking is on, and the return value would basically behave just
> > like any other libssh2 call. It would return `0` when it was a success.
> > Normal libssh2 behavior. If 0 is returned, then the second parameter is
> set
> > to a pointer to a single channel that is ready to be read, or NULL if
> none
> > are ready.
>
> I suppose that libssh2 maintains a queue of packets for various channels,
> and
> your proposed function would return the first item from the queue? Sounds
> good
> if you want to process all packets in order, but what if you want to skip
> some
> channels because their corresponding sockets are not ready to accept more
> data, and you want libssh2 to save the data for later, while blocking that
> channel?
>
> You might think, "why not save such data in a buffer instead?", but that's
> not
> the same thing. If you keep reading data which you can't send, libssh2 will
> adjust the window and accept more and more, and then you might run out of
> memory trying to buffer it all.
>

Hm, my thought process behind the API was this would allow libssh2 to
simply read the next SSH packet off the wire and return that channel.

If libssh2 were to return a list of ready channels, would it buffer these
all somewhere? Does libssh2 already do this internally (which would make
this easy)?

I'm not against the idea, I think it would be useful, I'm only thinking
about the implementation.

Best,
Mitchell

>
> > Again, like I said, I don't know the internal structure of libssh2 or
> > the feasibility of such a thing, but I can see looping through channels
> to
> > become overwhelming.
>
> Overwhelming? Well, the loop must be done in some way anyway inside the
> library. (I suppose that libssh2 maps from a channel ID to a struct
> pointer,
> and that might be a hash array lookup?)
>
> Again, see my point above about skipping channels which must be paused. One
> way would be to tell libssh2 that a particular channel shouldn't be
> checked.
> Another way would be to loop and check selectively.
>
> I'm quite happy with libssh2 as it is now, but I'd also like a nicer way to
> check a channel. Right now I have a helper routine which looks like this:
>
> static long _try_channel_read(... LIBSSH2_CHANNEL *channel)
> {
> long read_size;
>
> /* ugly kludge */
> libssh2_session_set_blocking(session->ssh_session, 0);
>
> read_size = libssh2_channel_read(channel,
> session->io_buffer,
> session->io_buffer_capacity);
> ...
>
> /* ugly kludge */
> libssh2_session_set_blocking(session->ssh_session, 1);
>
> return read_size;
> }
>
> --
> Best regards,
> Tom Weber
> Cryptzone Group AB
>
> _______________________________________________
> libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
>

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