Subject: Re: [PATCH] libssh2_channel_request_auth_agent

Re: [PATCH] libssh2_channel_request_auth_agent

From: Mitchell Hashimoto <mitchell.hashimoto_at_gmail.com>
Date: Sat, 17 Mar 2012 10:34:27 -0700

Peter,

On Sat, Mar 17, 2012 at 10:31 AM, Peter Stuge <peter_at_stuge.se> wrote:
> 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.
>

Thanks I'll take a look at that.

>
>> +++ 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.

Sorry, I really just copied the ssh2_exec.c example, which has all
these same problems. I'll fix man and perhaps in the future I'll send
another patch to fix up ssh2_exec as well.

I'll address the things you pointed out.

Thanks!
Mitchell

>
>
> //Peter
> _______________________________________________
> 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-17