[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Aleksandar,
I will lie if I never though of that possibility. I have even though of rewriting libssh in C++ using a decent OOP model.
But there is the problem of dependancies which unfortunately are really a problem for a small project like libssh. If we play well, libssh could be integrated in major IO toolkits like glibnet, QT, apache IO library, others, ...
Doing the bindings to glib will only make a few people happy and will scare to hell the other people who don't want to force users to install glib only for ssh support (Ask anyone using libssh on windows).
I think we need to find the lowest denominator on all IO main loop providers and provind the appropriate binding for them, while providing our own simplified main loop for people wanting something simple. In opposition to what you told, I think it is not that hard to integrate to other main loop while still providing all capabilities.
Others, please tell me what you think. I really feel that putting a hard dependance on glib at this state of the development will not help us.
About your idea of implementation : I feel we are thinking in the same direction, even if I think your SSH_POLL should be merged with the already existing struct ssh_socket.
Do you have somewhere near hand the specifications of the minimal interfaces to be provided to glib for a new network layer ? We'd do a top-down redesign to implement the interfaces to be sure to be compatible, while providing our own main loop system. We will then think on how to convert the existing code to the new interface, little bit by little bit while maintaining the old API integrity. It's important because otherwise incompatible code in a cousin branch will soon be incompatible with the main branch (as we did with the -nonblocking branch years ago).
I really count on your previous patch with the several modifications I told you about in my last mail. If needed we'll modifiy it again later. 

Thanks for your contribution,

Aris

Aleksandar Kanchev a écrit :

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@xxxxxxxxxxx
http://www.cerkinfo.be/cgi-bin/mailman/listinfo/libssh



------------------------------------------------------------------------

_______________________________________________
Libssh mailing list
Libssh@xxxxxxxxxxx
http://www.cerkinfo.be/cgi-bin/mailman/listinfo/libssh
Archive administrator: postmaster@lists.cynapses.org