Re: [PATCH] socket: do not enable POLLOUT for empty out buffer

[Nikolay <nitro@xxxxxxxxxxx>]

Hello, Aris.

> Normally the API is ok to use in non blocking mode, actually it makes no
> sense to use blocking/synchronous APIs in an asynchronous design.
> I just looked at the code again and I have no idea why it even mentions
> blocking mode. I think it's because every synchronous API calls you will
> make on a session you have added to an event handle will impact the
> other sessions in that same event handle, because if libssh has to do a
> synchronous recv() on SSH session 1, it has to run ssh_event_dopoll() on
> the event structure that contains ssh sessions 1 and 2.
> If your code is properly using callbacks it should work.
= I failed to do it with callbacks & non-blocking mode.

The scenario is:

session = ssh_new();
*ssh_set_blocking(session, 0);**
*ssh_event_add_session(eventloop, session);
ssh_options_set(session, ...) ...;
struct ssh_callbacks_struct cb;
memset(&cb, 0, sizeof(ssh_callbacks_struct));
cb.userdata = this;
cb.connect_status_function = cb_connect_status_function;
cb.auth_function = cb_auth_function;
cb.log_function = cb_log_function;
ssh_callbacks_init(&cb);
ssh_set_callbacks(session, &cb);
ssh_connect(session);
...
while (1) {
  ssh_event_dopoll(eventloop, 500);
}

Log output is:
09:10:54.710 cb_log_callback: ssh_connect: libssh 0.7.0 (c) 2003-2014 
Aris Adamantiadis, Andreas Schneider, and libssh contributors. 
Distributed under the LGPL, please refer to COPYING file for information 
about your rights, using threading threads_noop
09:10:54.710 cb_log_callback: ssh_socket_connect: Nonblocking connection 
socket: 23
09:10:54.710 cb_connect_status_function: 0.200000
09:10:54.710 cb_log_callback: ssh_connect: Socket connecting, now 
waiting for the callbacks to work
09:10:54.710 cb_log_callback: socket_callback_connected: Socket 
connection callback: 1 (0)

And that's it. No more calls to my callbacks.
If I make the session blocking:
*ssh_set_blocking(session, 1);**
*
Then it gets done right:

09:13:27.617 cb_log_callback: ssh_socket_connect: Nonblocking connection 
socket: 23
09:13:27.617 cb_connect_status_function: 0.200000
09:13:27.617 cb_log_callback: ssh_connect: Socket connecting, now 
waiting for the callbacks to work
09:13:27.617 cb_log_callback: socket_callback_connected: Socket 
connection callback: 1 (0)
09:13:27.621 cb_connect_status_function: 0.400000
09:13:27.621 cb_log_callback: ssh_client_connection_callback: SSH server 
banner: SSH-2.0-OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8
09:13:27.621 cb_log_callback: ssh_analyze_banner: Analyzing banner: 
SSH-2.0-OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8
09:13:27.621 cb_log_callback: ssh_analyze_banner: We are talking to an 
OpenSSH client version: 6.6 (60600)
09:13:27.621 cb_connect_status_function: 0.500000
09:13:27.622 cb_connect_status_function: 0.600000
09:13:27.623 cb_connect_status_function: 0.800000
09:13:27.666 cb_log_callback: ssh_packet_dh_reply: Received SSH_KEXDH_REPLY
09:13:27.676 cb_log_callback: ssh_client_curve25519_reply: 
SSH_MSG_NEWKEYS sent
09:13:27.676 cb_log_callback: ssh_packet_newkeys: Received SSH_MSG_NEWKEYS
09:13:27.677 cb_log_callback: ssh_packet_newkeys: Signature verified and 
valid
09:13:27.677 cb_connect_status_function: 1.000000

But in blocking mode during ssh_connect() call all other sockets are not 
getting their callbacks executed. I also need to check something 
periodically in main loop, so blocking mode doesn't seem like an option 
for me.

Am I doing something wrong way?

Best regards, Nikolay Karikh.


> On 2/08/17 17:14, Nikolay wrote:
> > Thank you, Aris.
> >
> > > No problem, as I understand you're using revents as a kind of a flag to
> > > detect when the output buffer is totally empty. It's not exactly how it
> > > was designed :)
> > = No, I do understand that POLLOUT is either a
> > connect-attempt-finished or outgoing-buffer-available event.
> > This is why I suppose the patch - in my mind we don't need to wait for
> > POLLOUT if we have already passed all the data to send(). I do not
> > quite understand the reason why we need to wait for POLLOUT
> > unconditionally...
> > POLLOUT will only mean that there is space available in output buffer.
> The idea is not that we "wait", but we pass the flag to the kind of
> events we're interested in. Usually on the next main loop iteration,
> poll returns with POLLOUT and we can set write_wont_block=1. We do this
> in order to, in future, be able to send() the next packet without having
> to wait for a main loop iteration. In the end we do the same amount of
> poll operations. We cannot send() anything without being sure the write
> operation will not block.
>
> > But I do not analyze revent value in socket callback function. I
> > simply check if session is established and do ssh_channel_select() or
> > ssh_connect()/ssh_auth...() otherwise.
> We probably need better ways of querying the ssh session to know in what
> state it is (connecting/authenticating/disconnecting/errored)
> > > I'm not sure I fully understand the sequence of actions here. Are we
> > > somewhere in (backtrace)
> > > ssh_event_dopoll()
> > > ____ssh_socket_event_callback()
> > > ________ssh_channel_select()
> > = Yes, backtrace seems like the one I have in mind.
> >
> > > Are the parameters to these functions SSH sessions? Because if so you
> > > may enter in reentrancy zone and you may have unexpected results.
> > = I only pass session socket to ssh_event_dopoll().
> > But you are possibly correct, as ssh_channel_select() seems to be
> > working with the channels' sessions' sockets.
> Actually from inside certain callbacks, some ssh functions will not
> work, like functions that expect reading something additional from the
> network. A good example is ssh_channel_open() in blocking mode.
> > > The best way to handle the spurious POLLOUT is to add the current ssh
> > > session into an ssh_event
> > > LIBSSH_API int ssh_event_add_session(ssh_event event, ssh_session
> > > session);
> > > and then call ssh_event_dopoll() with timeout=0
> > >
> > > I'm not sure how it would fit into your architecture though. Maybe you
> > > should use the channel callbacks instead of ssh_channel_select() inside
> > > of global socket callbacks.
> > Yes, I did try to use ssh_event_add_session() and set some callbacks.
> > But I strongly need it all to work in non-blocking mode. But the
> > documentation for ssh_event_add_session() says:
> > > remove the poll handle from session and assign them to a event, when
> > used in *blocking mode*
> Normally the API is ok to use in non blocking mode, actually it makes no
> sense to use blocking/synchronous APIs in an asynchronous design.
> I just looked at the code again and I have no idea why it even mentions
> blocking mode. I think it's because every synchronous API calls you will
> make on a session you have added to an event handle will impact the
> other sessions in that same event handle, because if libssh has to do a
> synchronous recv() on SSH session 1, it has to run ssh_event_dopoll() on
> the event structure that contains ssh sessions 1 and 2.
> If your code is properly using callbacks it should work.
> > I think it's the reason I failed with using ssh_event_add_session()
> > and session callbacks. I did get some SIGSEGV during those experiments.
> Maybe reentrancy issues? Send me a few backtraces, they can help.
> > I'm connecting in the following way:
> >
> > if (session_not_connected && ssh_connect() == SSH_AGAIN)
> >    return to eventloop and wait for event on session socket, then run
> > ssh_connect() again
> Looks fine. The best would be to use a session_connected callbacks that
> we still haven't implemented.
> > My thread should not block in waiting for particular
> > socket/session/channel I/O. I should be able to connect to multiple
> > ssh server in non-blocking async mode.
> I agree, libssh was designed with this use case in mind. But we have
> unfortunately probably missed a few corner cases because we haven't
> written programs that do this.
> > Best regards, Nikolay Karikh

Attachment: smime.p7s
Description: Криптографическая подпись S/MIME