On Mon, Mar 9, 2009 at 3:22 AM, Antoine Martin <[email protected]> wrote:
> Please consider this patch for merging.
Cool, thanks!
> Summary of the changes:
> * adds "--no-ipc" option for turning off unix domain socket listener.
1) This seems misnamed. There are many forms of IPC besides unix
domain sockets... one of them is TCP :-).
2) Can you explain the justification for why it is needed or useful?
> * adds "--tcp" option for turning on the TCP listener.
> * adds "--tcp-offset" option to change the TCP port number offset
> (defaults to 16000)
> * adds "--host" option to change the host that the TCP listener will
> bind to (defaults to 127.0.0.1)
> * adds "--port" option to override the offset/displayNo and specify the
> port number directly. (1)
> * adds support for "tcp:[portNumber]" syntax for the client.
> Which will connect to a TCP socket. (using the switches above)
Thinking about this a bit more, it seems to me that there are two
possible UIs that make sense:
Option A: No offsets, default ports, or any of that machinery at all
-- just have one switch that enables tcp listening, and requires a
port specification (and optionally host). Syntax something like:
xpra start :30 --bind-tcp :12345
xpra start :30 --bind-tcp 127.0.0.1:12345
xpra connect tcp:127.0.0.1:12345
Pros: no "magic", everything is very explicit
Cons: users have to keep track of both the display number (for running
X clients) and the TCP port number (for attaching).
Option B: There is a fixed offset, and the tcp listen port is computed
by combining that with the display number. If you want a different
port, then use a different display. (Since display numbers are
arbitrary anyway, and you're already choosing both the display number
and tcp port at the same time, i.e. startup, this is not really a
burden.) Syntax something like:
xpra start :30 --tcp
xpra start :30 --tcp --tcp-host 127.0.0.1
xpra connect tcp:127.0.0.1:30
Pros: only one number to keep track of, used for all purposes.
Cons: lots of magic, plus, umm... decreased lsof'ability, I guess. Not
sure I followed that part.
Do you agree? The current UI in your patch seems like a compromise
that ends up with more complexity than either, and many of the
disadvantages of both. I think we should pick one or the other and
stick to it.
As to which to pick... laid out like this, I can see the argument for
Option A better than before, but I still don't understand your use
case or why it is so much better for you, so I don't see how to make
an informed decision. Also, the systems programmer in me is crying out
that by the time one is using lsof as an automated tool, something has
gone seriously wrong already, and maybe we should fix *that*... like
maybe there should be a way to ask a server what port it is listening
on? I don't know...
Can you give any more details about what exactly you're trying to do?
Maybe post one or two of these scripts so we can see what's so hard?
(Sorry to give you such a hassle here, but I just don't feel
comfortable accepting code without understanding the rationale. And
you shouldn't feel comfortable either, because if I don't understand
then I'll probably break it later without realizing :-).
---
Specific comments on the patch:
Your code contains tab characters. These are fairly evil in general,
but in Python they're outright dangerous... please remove.
> + port=None
^^ please follow existing code style; in this case, by placing spaces
around operators.
> - def __init__(self, socketpath, clobber):
> + def __init__(self, socketpath, clobber, use_ipc, use_tcp, host, port):
^^ I would prefer XpraServer.__init__ to be defined like:
def __init__(self, socketpath, clobber, bound_sockets):
for sock in bound_sockets:
sock.listen(5)
gobject.io_add_watch(sock, gobject.IO_IN, self._new_connection)
with the actual option handling and socket binding code moved out to
the startup script; this is getting really tightly coupled.
---
Overall, it looks good -- just some tweaks plus a decision about the
UI, and it should be good to go in!
-- Nathaniel
_______________________________________________
Parti-discuss mailing list
[email protected]
http://lists.partiwm.org/cgi-bin/mailman/listinfo/parti-discuss
