-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 Nathaniel Smith wrote: > 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 :-). Yes: "The correct standard POSIX term is POSIX Local IPC Sockets" I guess I went a bit far when trimming: - --no-posix-local-ipc-socket ;) Irrelevant now anyway:
> 2) Can you explain the justification for why it is needed or useful? My justification was testing a tcp-only setup, but I can't think of a reason in real life. Will remove. >> * 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). I think that works. > 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. I don't like the scary magic either! > 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... Sure it's not nice, but I was thinking that if the local ipc socket is not accessible and the tcp socket is, it is nice to be able to go straight to it (without having to subsctract magic numbers to get there) > 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 :-). They are far too horrid to post! ;) Anyway, I think we agree now, so I'll resend a patch based on option A. > > --- > > Specific comments on the patch: > > Your code contains tab characters. These are fairly evil in general, Matter of opinion... > but in Python they're outright dangerous... Don't know, I'm new to python. > please remove. OK. > >> + port=None > ^^ please follow existing code style; in this case, by placing spaces > around operators. OK. > >> - 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. Will do. > > --- > > Overall, it looks good -- just some tweaks plus a decision about the > UI, and it should be good to go in! Great! Thanks Antoine -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEAREKAAYFAkm2IcwACgkQGK2zHPGK1rvrVACfTJ2uXFegCuMQImv+fyvCL0MR MOwAmgKUANKVOGtkgRzkLjjB4TZviaGR =oJyZ -----END PGP SIGNATURE----- _______________________________________________ Parti-discuss mailing list [email protected] http://lists.partiwm.org/cgi-bin/mailman/listinfo/parti-discuss
