code review 156109: Initial exp/draw/x11 implementation.

Initial exp/draw/x11 implementation.

This provides an experimental X11 backend for the exp/draw interface.
It does not aim to provide a complete implementation of the X11 client protocol.

This works for me (Ubuntu Hardy 8.04, GOARCH=386). Your mileage my vary.

[nigeltao, 2009-11-20 13:30:00]

Hello r, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review the following change.

[r, 2009-11-20 20:47:01]

minor comments

http://codereview.appspot.com/156109/diff/8/10
File src/pkg/exp/draw/x11/auth.go (right):

http://codereview.appspot.com/156109/diff/8/10#newcode13
src/pkg/exp/draw/x11/auth.go:13: func getDisplay() string {
all these functions could use a one-line introductory comment

http://codereview.appspot.com/156109/diff/8/10#newcode28
src/pkg/exp/draw/x11/auth.go:28: _, err := io.ReadFull(r, b);
io.ReadFull(r, b[0:2]) maybe?  it's done below but feels correct here.

http://codereview.appspot.com/156109/diff/8/10#newcode32
src/pkg/exp/draw/x11/auth.go:32: return uint16(b[0])*256 + uint16(b[1]), nil;
i prefer <<8 to *256

http://codereview.appspot.com/156109/diff/8/10#newcode47
src/pkg/exp/draw/x11/auth.go:47: func readAuth() (string, string) {
name the arguments, then you can just return without "","" everywhere

http://codereview.appspot.com/156109/diff/8/10#newcode51
src/pkg/exp/draw/x11/auth.go:51: r, err :=
os.Open(os.Getenv("HOME")+"/.Xauthority", os.O_RDONLY, 0444);
maybe check os.Getenv gives you something non-empty first.

http://codereview.appspot.com/156109/diff/8/11
File src/pkg/exp/draw/x11/conn.go (right):

http://codereview.appspot.com/156109/diff/8/11#newcode47
src/pkg/exp/draw/x11/conn.go:47: // (in response to an expose event) both want
to call FlushImage.
i would consider running having FlushImage run a goroutine under the covers and
send it messages.

http://codereview.appspot.com/156109/diff/8/11#newcode192
src/pkg/exp/draw/x11/conn.go:192: // Presume that the authentication protocol is
"MIT-MAGIC-COOKIE-1".
assume not presume.

http://codereview.appspot.com/156109/diff/8/11#newcode224
src/pkg/exp/draw/x11/conn.go:224: _, err := io.ReadFull(r, b);
again, better to slice here and below

http://codereview.appspot.com/156109/diff/8/11#newcode254
src/pkg/exp/draw/x11/conn.go:254: func checkPixmapFormats(r io.Reader, b []byte,
n int) (bool, os.Error) {
agree bool, err nil
code simplifies

http://codereview.appspot.com/156109/diff/8/11#newcode269
src/pkg/exp/draw/x11/conn.go:269: func checkDepths(r io.Reader, b []byte, n int,
visual uint32) (bool, os.Error) {
ditto

http://codereview.appspot.com/156109/diff/8/11#newcode306
src/pkg/exp/draw/x11/conn.go:306: func checkScreens(r io.Reader, b []byte, n
int) (uint32, uint32, os.Error) {
again

[nigeltao, 2009-11-21 08:14:58]

http://codereview.appspot.com/156109/diff/8/10
File src/pkg/exp/draw/x11/auth.go (right):

http://codereview.appspot.com/156109/diff/8/10#newcode13
src/pkg/exp/draw/x11/auth.go:13: func getDisplay() string {
On 2009/11/20 20:47:01, r wrote:
> all these functions could use a one-line introductory comment

Done.

http://codereview.appspot.com/156109/diff/8/10#newcode28
src/pkg/exp/draw/x11/auth.go:28: _, err := io.ReadFull(r, b);
On 2009/11/20 20:47:01, r wrote:
> io.ReadFull(r, b[0:2]) maybe?  it's done below but feels correct here.

Done.

http://codereview.appspot.com/156109/diff/8/10#newcode32
src/pkg/exp/draw/x11/auth.go:32: return uint16(b[0])*256 + uint16(b[1]), nil;
On 2009/11/20 20:47:01, r wrote:
> i prefer <<8 to *256

Done.

http://codereview.appspot.com/156109/diff/8/10#newcode47
src/pkg/exp/draw/x11/auth.go:47: func readAuth() (string, string) {
On 2009/11/20 20:47:01, r wrote:
> name the arguments, then you can just return without "","" everywhere

Done.

http://codereview.appspot.com/156109/diff/8/10#newcode51
src/pkg/exp/draw/x11/auth.go:51: r, err :=
os.Open(os.Getenv("HOME")+"/.Xauthority", os.O_RDONLY, 0444);
On 2009/11/20 20:47:01, r wrote:
> maybe check os.Getenv gives you something non-empty first.

Are you checking for HOME being accidentally unset, or maliciously unset?

For the former, you ought to get an I/O error anyway. For the latter, this
doesn't stop me setting HOME="/home/..".

http://codereview.appspot.com/156109/diff/8/11
File src/pkg/exp/draw/x11/conn.go (right):

http://codereview.appspot.com/156109/diff/8/11#newcode47
src/pkg/exp/draw/x11/conn.go:47: // (in response to an expose event) both want
to call FlushImage.
On 2009/11/20 20:47:01, r wrote:
> i would consider running having FlushImage run a goroutine under the covers
and
> send it messages.

Done. I'm not sure I've done the teardown properly, though.

http://codereview.appspot.com/156109/diff/8/11#newcode192
src/pkg/exp/draw/x11/conn.go:192: // Presume that the authentication protocol is
"MIT-MAGIC-COOKIE-1".
On 2009/11/20 20:47:01, r wrote:
> assume not presume.

Done.

http://codereview.appspot.com/156109/diff/8/11#newcode224
src/pkg/exp/draw/x11/conn.go:224: _, err := io.ReadFull(r, b);
On 2009/11/20 20:47:01, r wrote:
> again, better to slice here and below

Done.

http://codereview.appspot.com/156109/diff/8/11#newcode254
src/pkg/exp/draw/x11/conn.go:254: func checkPixmapFormats(r io.Reader, b []byte,
n int) (bool, os.Error) {
On 2009/11/20 20:47:01, r wrote:
> agree bool, err nil
> code simplifies

Done.

http://codereview.appspot.com/156109/diff/8/11#newcode269
src/pkg/exp/draw/x11/conn.go:269: func checkDepths(r io.Reader, b []byte, n int,
visual uint32) (bool, os.Error) {
On 2009/11/20 20:47:01, r wrote:
> ditto

Done.

http://codereview.appspot.com/156109/diff/8/11#newcode306
src/pkg/exp/draw/x11/conn.go:306: func checkScreens(r io.Reader, b []byte, n
int) (uint32, uint32, os.Error) {
On 2009/11/20 20:47:01, r wrote:
> again

Done.

[nigeltao, 2009-11-23 05:00:11]

http://codereview.appspot.com/156109/diff/1022/2012
File src/pkg/exp/draw/x11/auth.go (right):

http://codereview.appspot.com/156109/diff/1022/2012#newcode61
src/pkg/exp/draw/x11/auth.go:61: return
We now check for an empty $HOME variable.

[r, 2009-11-23 22:41:54]

http://codereview.appspot.com/156109/diff/1022/2013
File src/pkg/exp/draw/x11/conn.go (right):

http://codereview.appspot.com/156109/diff/1022/2013#newcode36
src/pkg/exp/draw/x11/conn.go:36: gc, window, root, visual	uint32;	// X resource
IDs.
type visual uint32 ??

[rsc, 2009-11-23 22:49:41]

took a quick look.
seems like a good start

http://codereview.appspot.com/156109/diff/1022/2013
File src/pkg/exp/draw/x11/conn.go (right):

http://codereview.appspot.com/156109/diff/1022/2013#newcode36
src/pkg/exp/draw/x11/conn.go:36: gc, window, root, visual	uint32;	// X resource
IDs.
I suspect you're going to want to introduce a 

type ID uint32

or some such for these, but uint32 is fine for now.

http://codereview.appspot.com/156109/diff/1022/2013#newcode52
src/pkg/exp/draw/x11/conn.go:52: // flusher runs in its own goroutine, servicing
both FlushImage calls directly from the exp/draw client
s/servicing/serving/

http://codereview.appspot.com/156109/diff/1022/2013#newcode157
src/pkg/exp/draw/x11/conn.go:157: keysym = -keysym
Just as an FYI, I don't know that the negative keysym
for up event is such a great idea.  It was just a hack to
get spacewar working.  Almost everything in the draw
interface is up for debate.

http://codereview.appspot.com/156109/diff/1022/2013#newcode470
src/pkg/exp/draw/x11/conn.go:470: setU32LE(c.buf[0:4], 0x00060037);	// 0x37 is
the CreateGC opcode, and the message is 6 x 4 bytes long.
I wonder if you'd be better off using encoding/binary
with a struct that had these fields in it.

[nigeltao, 2009-11-24 13:12:57]

http://codereview.appspot.com/156109/diff/1022/2013
File src/pkg/exp/draw/x11/conn.go (right):

http://codereview.appspot.com/156109/diff/1022/2013#newcode36
src/pkg/exp/draw/x11/conn.go:36: gc, window, root, visual	uint32;	// X resource
IDs.
On 2009/11/23 22:49:41, rsc wrote:
> I suspect you're going to want to introduce a 
> 
> type ID uint32
> 
> or some such for these, but uint32 is fine for now.

Done, but I'm not sure if the extra casts were a win.

http://codereview.appspot.com/156109/diff/1022/2013#newcode52
src/pkg/exp/draw/x11/conn.go:52: // flusher runs in its own goroutine, servicing
both FlushImage calls directly from the exp/draw client
On 2009/11/23 22:49:41, rsc wrote:
> s/servicing/serving/
> 

Done.

http://codereview.appspot.com/156109/diff/1022/2013#newcode470
src/pkg/exp/draw/x11/conn.go:470: setU32LE(c.buf[0:4], 0x00060037);	// 0x37 is
the CreateGC opcode, and the message is 6 x 4 bytes long.
On 2009/11/23 22:49:41, rsc wrote:
> I wonder if you'd be better off using encoding/binary
> with a struct that had these fields in it.
> 

Maybe... but I'm not convinced the extra indirection is worth it for this
one-off exchange of magic numbers. If we were writing a number of CreateWindow
requests, for example, then I'd probably be more willing to use a custom struct.

[r1, 2009-11-24 18:21:18]

LGTM

i'm happy enough with this for something in exp. if rsc agrees, check it in

[rsc, 2009-11-24 23:56:15]

i'm happy.

nigel - i added you to the committer list so you
can check these things in yourself (after review,
of course).  to do that, you need to tweak your
mercurial setup.  see http://www/~rsc/gohg.html
the account with permission is nigeltao@golang.org.

russ


On Tue, Nov 24, 2009 at 10:21,  <robpike@gmail.com> wrote:
> LGTM
>
> i'm happy enough with this for something in exp. if rsc agrees, check it
> in
>
> http://codereview.appspot.com/156109
>

[nigeltao, 2009-11-25 07:31:47]

*** Submitted as http://code.google.com/p/go/source/detail?r=4a3f8104324e ***

Initial exp/draw/x11 implementation.

This provides an experimental X11 backend for the exp/draw interface.
It does not aim to provide a complete implementation of the X11 client protocol.

This works for me (Ubuntu Hardy 8.04, GOARCH=386). Your mileage my vary.

R=r, rsc, r1
CC=golang-dev
http://codereview.appspot.com/156109