[Libssh] [PATCH 1/2] Add a generic way to handle sockets asynchronously.

[Aleksandar Kanchev]

Hi Aris,

sorry for my late response, it was a busy week(end). Thanks for your 
feedback!

I also think that the patch is too general and exports too much. The 
Idea was that we have 3 basic components:

   1. bind (server) / connect (client) - it just polls the socket for
      read/write until a client connects (ready for accept()) or if the
      client code connect()ed.
   2. version - simply send our version string and read the remote
      version string, check also if it's valid.
   3. packet - this simply reads / writes packets. It also encrypts or
      decrypts them based on the session settings. The packets which are
      read must also be forwarded to their handlers.

One SSH_POLL_CTX is created for each thread. It could be passed to one 
of the ssh_bind_*() functions to decide which thread will wait for new 
clients. The ssh_bind code could internally register a new SSH_POLL for 
the bind socket and add it to the SSH_POLL_CTX. The client code (think 
main()) would simply have to call ssh_poll_ctx() within it's main loop.

When a new client arrives, the internal callback of the bind code will 
be called and it would accept() the connection and probably create a 
SSH_SESSION object (not sure if it would be needed at that moment). Then 
the SSH_POLL_CONTEXT along with the client socket (or SSH_SESSION) could 
be passed to the version component.

The version component could register the client socket with it's own 
callback to the SSH_POLL_CTX, send libssh's version string and wait for 
the client's version string. Eventually the client string will arrive 
since the SSH_POLL_CTX is still being polled within the main() loop. It 
could be checked if it's supported, then the version component could 
free it's resources (for example the SSH_POLL object won't be needed 
anymore) and it could pass the socket along with the SSH_POLL_CTX to the 
packet layer. Probably it should create a new SSH_SESSION object before 
doing that.

The packet component could use the SSH_SESSION object to know how to 
handle newly received packets (encryption and handlers). It could be a 
combination of the current socket and packet components. The need for a 
separate socket layer in this architecture is very questionable since it 
would only be needed here. The version component could make some use of 
it but the version's job is so trivial that it would probably be an 
overkill.

After having these 3 basic components the rest of the code would be very 
straight forward: just implement the rest of the ssh protocol. This 
would be done by registering handlers for the supported packet types. 
The code already exist within libssh, but it probably needs to be 
rearranged.

So, this sounds very nice and easy if you're going to use libssh to 
simply implement a ssh client or server. But how about integrating it 
with a GUI application. It cannot really coexist with glib's main loop 
for example, since glib's loop could also be polling some sockets or 
file descriptors. A workaround would be to make the ssh_poll* code to 
integrate with glib's main loop. But this would make the poll code very 
complicated.

After thinking about that, why don't we simply use glib for libssh? My 
ssh_poll code is actually glib's main loop with glib's io channels, if 
you look closer. We could use glib's lists, buffers, malloc, io, 
threads, signals ... This would make libssh's code smaller and would 
allow us to concentrate on the ssh protocol itself and leave most of the 
rest to glib. Why reinvent the wheel?

Using glib's signals, for example, is a very common callback mechanism, 
exactly what you're looking for. It would also allow us to do complex 
things, like chaining the callbacks.

Also most of libssh's code looks like GObject (SSH_SESSION with all the 
set and get methods). Porting it to GObject would allow the use of all 
the glib bindings: python, perl ... Having glib as dependancy isn't a 
bad thing since most of the projects today use it. Doing "yum remove 
glib2" on my Fedora 12 adds 545 packages for removal.

So what I'm saying is, that we should throw my patch away and start 
working on a branch of libssh which is build on top of glib and gobject. 
This would probably break the API a lot, but the library is still in 
version 0.3 and besides, it's typical for an open source project to 
break the API when doing such a big change. If everything goes well you 
could start releasing the new branch as libssh 1.0. This would indicate 
the use of a new API.

Hopefully, you're not scared by this idea :)

best regards,
Aleks


On 06/25/2009 10:23 PM, Aris Adamantiadis wrote:
> Dear Alksandar,
>
> I have carefuly read your code and I'm now convinced you're going in
> the good direction.
>
> However, my global feeling is that what you did is too generalistic for
> libssh. I don't want libssh to be the tool used by our users as a main
> loop, because it will never be an optimal tool. We may provide basic
> functionnalities though.
>
> Also, it involves exposing more and more of the API, which also
> involves more headaches when we change something.
>
> Anyway, I like your patch. Here are my recommandations before I merge
> it in HEAD:
>
> 1/ the POLL* constants should be moved back in priv.h. We don't want to
> export them, because they might be already defined. Also the whole API
> should be moved in priv.h, because I think it should not be external.
> 2/ Please move your (very precise !) doxygen comments into the code
> instead of the headers, to follow the current practice.
> 3/ When I said your code was a little big too generalistic, I mostly
> though of the callback system. It's very well written but we don't need
> all of these features at this moment, because a static callback will be
> enough. I think the code which handle the incoming of data should be a
> method of the SSH_SESSION structure/class. See later how I see the user
> provided callback system.
> 4/ Is it possible to merge your POLL structure with the already
> existing socket object ? I think there is a 1-1 relation between them,
> and some of the already existing code in socket.c overlaps with yours.
> 5/ The POLL_CTX name is maybe not the right one. Firstly, after the
> POLL/ssh_socket merge, the name will mismatch, but the "CTX" part
> doesn't make it obvious to me that we are dealing with a global socket
> polling system.
> Also, I would like to respect the demeter's law regarding the user
> codes. Demeter tells that you should not call a method of an object
> which is member of yet another object. The could I would like to avoid
> (for users) is:
>
> ssh_poll_ctx_add(ssh_get_socket(session),...);
> so, this function should take SSH_SESSION's as parameters. (Obviously,
> I have nothing against an internal function which only takes
> ssh_sockets as parameters).
> If we insist into provinding callback to users for their own sockets,
> I'd see a simple function doing
> ssh_(choosename)_add_socket(CTX *, socket_t usersocket, callback_t
> callback, void *user);
> with callback_t being a function pointer taking a socket_t, a void *
> and a pollevents as parameters.
>
> We would wrap ourselves the socket into a ssh_socket and so on. The
> goal is to provide as few API as possible while still providing the
> maximum of possibilities.
>
> My view is still a little fuzzy (as you might see, my mail is not clear
> as crystal and I changed my mind several times while writing), so
> please discuss what you find relevant.
>
> Thanks again for prividing us your patch, and please excuse me for the
>   >79 chars/line layout, I have yet to understand why thunderbird
> doesn't split the lines as it used to do.
>
> Regards,
>
> Aris
>
> _______________________________________________
> Libssh mailing list
> Libssh at cerkinfo.be
> http://www.cerkinfo.be/cgi-bin/mailman/listinfo/libssh
>    

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.cerkinfo.be/pipermail/libssh/attachments/20090702/a805011d/attachment.htm