Bazaar

Code review comment for lp:~jameinel/bzr/2.3-bzr-connect-ssh

2.3-bzr-connect-ssh
Merge into bzr.dev

Revision history for this message

John A Meinel (jameinel) wrote on 2010-09-02:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...
>
> This connection should not start processing the request (the ssh command here)
> before the key exchange. I.e TestingSFTPConnectionHandler.handle() which is
> called after setup() should not start before the key exchange is done). The sleep
> was enough... in some cases. But the join() *ensures* the synchronisation.
>
> So:
>
> 28 server = tcs._server_interface(tcs)
> 29 + # This blocks until the key exchange has been done
> 30 ssh_server.start_server(None, server)
>
> is misleading too, that's the join() that guarantees that the key exchange
> has occurred and that it's safe to process the request. This comment applies
> to the .join() indeed.

No. The 'start_server' waits until the key negotiation has occurred.
After that point it does the actual communication on the channel.

The 'ssh_server.join()' waits all the way until the remote end
disconnects and everything has completed.

The reason this is 'okay' is because the .handle() method is *completely
empty*, and is just called immediately after .setup().

The BaseResponseHandler __init__ code is:

self.setup()
self.handle()
self.close() # or finish or whatever

>
> I think that one confusing point is that ssh presents an encrypted medium
> (paramiko.Transport) from which one or several "connections" (in the TCP sense)
> can happen (paramiko.Channel which can be used a socket).
> We don't really care about the distinction in our case and view the Transport + Channel
> as a single socket that appears unencrypted to us.
>
> What this means is that we shouldn't touch the socket before the key exchange has occurred
> and your fix ensures that.

That is true, but still what I said above is correct.

>
> Anyway, thank you for finding this nasty one, we may still want to avoid spawning
> a subprocess here though and clean up some of the tearDown done here which I'm pretty
> sure is now useless.
>

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkx/qH0ACgkQJdeBCYSNAAPylQCfWmEOqi3FV7Iatw6ueNi0D3tH
pkoAoIrqn59BYHg6kRojSNotoI2c4RJT
=UPuM
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...
> 
> This connection should not start processing the request (the ssh command here)
> before the key exchange. I.e TestingSFTPConnectionHandler.handle() which is 
> called after setup() should not start before the key exchange is done). The sleep
> was enough... in some cases. But the join() *ensures* the synchronisation.
> 
> So:
> 
> 28	 server = tcs._server_interface(tcs)
> 29	+ # This blocks until the key exchange has been done
> 30	ssh_server.start_server(None, server)
> 
> is misleading too, that's the join() that guarantees that the key exchange
> has occurred and that it's safe to process the request. This comment applies
> to the .join() indeed.

No. The 'start_server' waits until the key negotiation has occurred.
After that point it does the actual communication on the channel.

The 'ssh_server.join()' waits all the way until the remote end
disconnects and everything has completed.

The reason this is 'okay' is because the .handle() method is *completely
empty*, and is just called immediately after .setup().

The BaseResponseHandler __init__ code is:

self.setup()
 self.handle()
 self.close() # or finish or whatever

> 
> I think that one confusing point is that ssh presents an encrypted medium
> (paramiko.Transport) from which one or several "connections" (in the TCP sense) 
> can happen (paramiko.Channel which can be used a socket).
> We don't really care about the distinction in our case and view the Transport + Channel
> as a single socket that appears unencrypted to us.
> 
> What this means is that we shouldn't touch the socket before the key exchange has occurred
> and your fix ensures that.

That is true, but still what I said above is correct.

> 
> Anyway, thank you for finding this nasty one, we may still want to avoid spawning
> a subprocess here though and clean up some of the tearDown done here which I'm pretty
> sure is now useless.
>

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkx/qH0ACgkQJdeBCYSNAAPylQCfWmEOqi3FV7Iatw6ueNi0D3tH
pkoAoIrqn59BYHg6kRojSNotoI2c4RJT
=UPuM
-----END PGP SIGNATURE-----

« Back to merge proposal