Issues with channel callbacks

[Tilo Eckert <tilo.eckert@xxxxxxx>]

Hi,

I am experimenting with the ssh_event/connector APIs to remotely execute
commands and process the returned output via callbacks (to retain the
order of stdin and stderr messages as sent by the server). I used the
shell() and select_loop() functions from the ssh_client.c example as a
starting point. However, I replaced the stdout and stderr connectors
with callbacks like this:

> ssh_connector connector_in;
> ssh_callbacks_init(&callback);
> callback.userdata = NULL;
> callback.channel_data_function = channel_data_callback;
> callback.channel_close_function = channel_close_callback;
> callback.channel_eof_function = channel_eof_callback;
> callback.channel_exit_signal_function = channel_exit_signal_callback;
> callback.channel_exit_status_function = channel_exit_status_callback;
> ssh_add_channel_callbacks(channel, &callback);

I identified multiple issues with these callbacks during my experiments
and would appreciate some feedback on potential solutions:

1) If my data callback consumes less than the provided number of bytes
and the server closes the channel, the channel_rcv_close() callback in
channels.c will not actually set the channel state to closed, but set
channel->delayed_close = 1 instead because channel buffers are not
empty. This leads to an infinite loop in the main event loop:
> while (ssh_channel_is_open(channel)) {
>     ssh_event_dopoll(event, 60000);
> }
Question: The assignment channel->delayed_close = 1 seems to be there
for a long time. Is there still any justification for it? The only thing
it seems to do is prevent writing to the channel, which also happens
when the channel is in closed state. If it was meant to delay setting
the closed state on the channel until remaining buffer contents have
been read via ssh_channel_read(), I would expect that the channel is set
to closed state in one of the read functions, but I cannot find such
code. That leads me to believe that channel->delayed_close can be safely
removed to fix the infinite loop. I might be wrong, though.

2) If the channel is being closed and my data callback did not consume
all data before, I do not get a chance to process the remaining data
because there simply exists no callback to handle this case. I would
like to introduce a new callback for this purpose which is executed in
channel_rcv_close() right before the close callback if there is data
remaining in the buffers.

3) My data callback is executed exactly once for every received channel
data packet. I think this should be changed to be more user-friendly:
The data callback should be executed repeatedly until it consumed all
available bytes or 0 (i.e. the callback signals that there is not enough
data to process). That would make it easier to implement custom protocol
handlers via callbacks. For example, a data callback that processes a
single line of text per call would no longer require a loop in the
callback function, leading to code that is easier to read.

Regards,
Tilo Eckert