<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=us-ascii" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Hi Aris,<br>
<br>
sorry for my late response, it was a busy week(end). Thanks for your
feedback!<br>
<br>
I also think that the patch is too general and exports too much. The
Idea was that we have 3 basic components:<br>
<ol>
<li>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.</li>
<li>version - simply send our version string and read the remote
version string, check also if it's valid.</li>
<li>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.</li>
</ol>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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?<br>
<br>
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. <br>
<br>
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.<br>
<br>
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.<br>
<br>
Hopefully, you're not scared by this idea :)<br>
<br>
best regards,<br>
Aleks<br>
<br>
<br>
On 06/25/2009 10:23 PM, Aris Adamantiadis wrote:
<blockquote cite="mid:4A43DCDC.3050407@0xbadc0de.be" type="cite">
<pre wrap="">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
<a class="moz-txt-link-abbreviated" href="mailto:Libssh@cerkinfo.be">Libssh@cerkinfo.be</a>
<a class="moz-txt-link-freetext" href="http://www.cerkinfo.be/cgi-bin/mailman/listinfo/libssh">http://www.cerkinfo.be/cgi-bin/mailman/listinfo/libssh</a>
</pre>
</blockquote>
<br>
</body>
</html>