Subject: Re: [PATCH] libssh2_channel_request_auth_agent

Re: [PATCH] libssh2_channel_request_auth_agent

From: Peter Stuge <peter_at_stuge.se>
Date: Sat, 17 Mar 2012 18:31:27 +0100

Mitchell Hashimoto wrote:
> +int
> +libssh2_channel_request_auth_agent(LIBSSH2_CHANNEL *channel);
> +
> +.SH DESCRIPTION
> +Request that agent forwarding be enabled for this SSH session. This sends the
> +request over this specific channel, which causes the agent listener to be
> +started on the remote side upon success. This agent listener will then run
> +for the duration of the SSH session.
> +
> +\fIchannel\fP - Previously opened channel instance such as returned by
> +.BR libssh2_channel_open_ex(3)

This doesn't make any sense to me. Model like the existing API which
create a special purpose channel, and reuse their infrastructure,
specifically libssh2_channel_process_startup(). See libssh2.h about
libssh2_channel_subsystem.

> +++ b/example/ssh2_agent_forwarding.c
> @@ -0,0 +1,331 @@
..
> +int main(int argc, char *argv[])
> +{
> + const char *hostname = "127.0.0.1";
> + const char *commandline = "uptime";

Suggest commandline "ssh-add -l" which will actually show the agent
forwarding being used.

> + /* tell libssh2 we want it all done non-blocking */
> + libssh2_session_set_blocking(session, 0);

Move this call to much later..

> + while ((rc = libssh2_session_handshake(session, sock)) ==
> + LIBSSH2_ERROR_EAGAIN);
..
> + while ((rc = libssh2_userauth_password(session, username, password)) ==
> + LIBSSH2_ERROR_EAGAIN);
..
> + while ((rc = libssh2_userauth_publickey_fromfile(session, username,
> + "/home/user/"
> + ".ssh/id_rsa.pub",
> + "/home/user/"
> + ".ssh/id_rsa",
> + password)) ==
> + LIBSSH2_ERROR_EAGAIN);
..
> + while( (channel = libssh2_channel_open_session(session)) == NULL &&
> + libssh2_session_last_error(session,NULL,NULL,0) ==
> + LIBSSH2_ERROR_EAGAIN )
> + {
> + waitsocket(sock, session);
> + }
..
> + while( (rc = libssh2_channel_request_auth_agent(channel)) ==
> + LIBSSH2_ERROR_EAGAIN )
> + {
> + waitsocket(sock, session);
> + }
..
> + while( (rc = libssh2_channel_exec(channel, commandline)) ==
> + LIBSSH2_ERROR_EAGAIN )
> + {
> + waitsocket(sock, session);
> + }
..

So that you do not need these very ugly loops!

> + for( ;; )
> + {
> + /* loop until we block */
> + int rc;
> + do
> + {
> + char buffer[0x4000];
> + rc = libssh2_channel_read( channel, buffer, sizeof(buffer) );
> + if( rc > 0 )
> + {
> + int i;
> + bytecount += rc;
> + fprintf(stderr, "We read:\n");
> + for( i=0; i < rc; ++i )
> + fputc( buffer[i], stderr);
> + fprintf(stderr, "\n");
> + }
> + else {
> + if( rc != LIBSSH2_ERROR_EAGAIN )
> + /* no need to output this for the EAGAIN case */
> + fprintf(stderr, "libssh2_channel_read returned %d\n", rc);
> + }
> + }
> + while( rc > 0 );
> +
> + /* this is due to blocking that would occur otherwise so we loop on
> + this condition */
> + if( rc == LIBSSH2_ERROR_EAGAIN )
> + {
> + waitsocket(sock, session);
> + }
> + else
> + break;
> + }

In fact, there is no reason at all to set non-blocking in this
example. It makes the code very ugly and is completely unneccessary.

> + while( (rc = libssh2_channel_close(channel)) == LIBSSH2_ERROR_EAGAIN )
> + waitsocket(sock, session);

Another ugly loop. Please get rid of all of them.

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