Issue 183111: code review 183111: Add authentication. (Closed)
DescriptionAdd authentication.
Other code fixing:
- Fixed bugs in get32.
- Fix code for parsing display string (as a new function).
- Fix code for connecting to X server. The old code only work
if the server is listening to TCP port, otherwise it doesn't
work (at least in my PC).
Patch Set 1 #Patch Set 2 : code review 183111: Add authentication. #
Total comments: 23
Patch Set 3 : code review 183111: Add authentication. #Patch Set 4 : code review 183111: Add authentication. #
Total comments: 15
Patch Set 5 : code review 183111: Add authentication. #
Total comments: 6
Patch Set 6 : code review 183111: Add authentication. #
Total comments: 61
Patch Set 7 : code review 183111: Add authentication. #
Total comments: 2
Patch Set 8 : code review 183111: Add authentication. #
Total comments: 4
Patch Set 9 : code review 183111: Add authentication. #Patch Set 10 : code review 183111: Add authentication. #
Total comments: 2
Patch Set 11 : code review 183111: Add authentication. #
Total comments: 5
Patch Set 12 : code review 183111: Add authentication. #
Total comments: 2
Patch Set 13 : code review 183111: Add authentication. #MessagesTotal messages: 37
Hello nigeltao_golang (cc: golang-dev@googlegroups.com), I'd like you to review the following change.
Sign in to reply to this message.
I'm not 100% familiar with how xgb works, so I'll let Tor override me if he wants to. http://codereview.appspot.com/183111/diff/3/1002 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/3/1002#newcode316 src/pkg/xgb/xgb.go:316: return nil, os.NewError(fmt.Sprintf("cannot connect: %s", err)) Rather than a fmt.Sprintf, you can just + the two strings (the second one is err.String()). Same on line 326. http://codereview.appspot.com/183111/diff/3/1002#newcode323 src/pkg/xgb/xgb.go:323: if len(protocol) != 0 || strings.HasPrefix(host, "unix") { Why is the strings.HasPrefix(host, "unix") check here? If my machine name is unixisthebest, but the protocol is "", doesn't net.Dial just return an error? http://codereview.appspot.com/183111/diff/3/1002#newcode332 src/pkg/xgb/xgb.go:332: if len(file) > len(base)+20 { XCB's _xcb_open truncates at 20, but Go strings aren't C strings, so I wouldn't bother with the truncation. http://codereview.appspot.com/183111/diff/3/1002#newcode348 src/pkg/xgb/xgb.go:348: if len(authName) != 18 || len(authData) != 16 { This assumption was correct for my system (which connects over a Unix socket), but this might be too strict for Tor's system (which connects over a TCP socket, and IIUC doesn't require (or provide??) an authentication name/value). Tor -- what do you think? http://codereview.appspot.com/183111/diff/3/1002#newcode357 src/pkg/xgb/xgb.go:357: put16(buf[6:], 18) // length of authName I'd keep these as before, i.e. uint16(len(authName)) instead of 18, etcetera. Otherwise, you're probably breaking Tor's system. http://codereview.appspot.com/183111/diff/3/1002#newcode433 src/pkg/xgb/xgb.go:433: func parseDisplayString(dispStr string) (host, protocol, display, screen string, err os.Error) { I'd just call it parseDisplay, rather than parseDisplayString. Also, the documentation comment in the line above doesn't add anything. The best comment would probably give an example input (e.g. "unix/foobar:1.0") and output. Finally, if I've got the DISPLAY string syntax correct, I'd re-order protocol and host in the return values. http://codereview.appspot.com/183111/diff/3/1002#newcode435 src/pkg/xgb/xgb.go:435: if len(dispStr) == 0 { If you're changing xgb.Dial so that it can now take an empty string, please add a comment (to xgb.Dial). http://codereview.appspot.com/183111/diff/3/1002#newcode456 src/pkg/xgb/xgb.go:456: colonIdx++ No need to ++ it, just +1 on the next line. http://codereview.appspot.com/183111/diff/3/1002#newcode459 src/pkg/xgb/xgb.go:459: nums := strings.Split(dispStr, ".", 0) strings.Split seems excessive -- just do another LastIndex. If it's malformed, we'll catch it soon enough. http://codereview.appspot.com/183111/diff/3/1002#newcode473 src/pkg/xgb/xgb.go:473: func readAuthority(hostname, display string) (name, data string, err os.Error) { I'd keep the auth-related functions in a separate auth.go file. http://codereview.appspot.com/183111/diff/3/1002#newcode474 src/pkg/xgb/xgb.go:474: Unnecessary new line? http://codereview.appspot.com/183111/diff/3/1002#newcode491 src/pkg/xgb/xgb.go:491: err = os.NewError("searching Xauthority file: unknown home directory.") I'd just write "Xauthority not found: unknown HOME"
Sign in to reply to this message.
http://codereview.appspot.com/183111/diff/3/1002 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/3/1002#newcode316 src/pkg/xgb/xgb.go:316: return nil, os.NewError(fmt.Sprintf("cannot connect: %s", err)) Done. On 2010/01/05 12:56:21, nigeltao_golang wrote: > Rather than a fmt.Sprintf, you can just + the two strings (the second one is > err.String()). Same on line 326. http://codereview.appspot.com/183111/diff/3/1002#newcode323 src/pkg/xgb/xgb.go:323: if len(protocol) != 0 || strings.HasPrefix(host, "unix") { It is totally wrong. I've fixed it. The protocol needs a hostname, and the hostname can be anything but not empty. PTAL: there is one problem. If the display string only contains hostname and display, such as localhost:0.0, the connection created a socket file (with a same name as the hostname) but didn't delete it if the connection closes. On 2010/01/05 12:56:21, nigeltao_golang wrote: > Why is the strings.HasPrefix(host, "unix") check here? If my machine name is > unixisthebest, but the protocol is "", doesn't net.Dial just return an error? http://codereview.appspot.com/183111/diff/3/1002#newcode332 src/pkg/xgb/xgb.go:332: if len(file) > len(base)+20 { Fixed. On 2010/01/05 12:56:21, nigeltao_golang wrote: > XCB's _xcb_open truncates at 20, but Go strings aren't C strings, so I wouldn't > bother with the truncation. http://codereview.appspot.com/183111/diff/3/1002#newcode357 src/pkg/xgb/xgb.go:357: put16(buf[6:], 18) // length of authName Agreed. On 2010/01/05 12:56:21, nigeltao_golang wrote: > I'd keep these as before, i.e. uint16(len(authName)) instead of 18, etcetera. > Otherwise, you're probably breaking Tor's system. http://codereview.appspot.com/183111/diff/3/1002#newcode433 src/pkg/xgb/xgb.go:433: func parseDisplayString(dispStr string) (host, protocol, display, screen string, err os.Error) { Fixed. On 2010/01/05 12:56:21, nigeltao_golang wrote: > I'd just call it parseDisplay, rather than parseDisplayString. Also, the > documentation comment in the line above doesn't add anything. The best comment > would probably give an example input (e.g. "unix/foobar:1.0") and output. > Finally, if I've got the DISPLAY string syntax correct, I'd re-order protocol > and host in the return values. http://codereview.appspot.com/183111/diff/3/1002#newcode435 src/pkg/xgb/xgb.go:435: if len(dispStr) == 0 { I've revised the comment. On 2010/01/05 12:56:21, nigeltao_golang wrote: > If you're changing xgb.Dial so that it can now take an empty string, please add > a comment (to xgb.Dial). http://codereview.appspot.com/183111/diff/3/1002#newcode456 src/pkg/xgb/xgb.go:456: colonIdx++ Fixed. On 2010/01/05 12:56:21, nigeltao_golang wrote: > No need to ++ it, just +1 on the next line. http://codereview.appspot.com/183111/diff/3/1002#newcode459 src/pkg/xgb/xgb.go:459: nums := strings.Split(dispStr, ".", 0) Fixed. On 2010/01/05 12:56:21, nigeltao_golang wrote: > strings.Split seems excessive -- just do another LastIndex. If it's malformed, > we'll catch it soon enough. http://codereview.appspot.com/183111/diff/3/1002#newcode473 src/pkg/xgb/xgb.go:473: func readAuthority(hostname, display string) (name, data string, err os.Error) { Moved to auth.go. On 2010/01/05 12:56:21, nigeltao_golang wrote: > I'd keep the auth-related functions in a separate auth.go file. http://codereview.appspot.com/183111/diff/3/1002#newcode474 src/pkg/xgb/xgb.go:474: Fixed. On 2010/01/05 12:56:21, nigeltao_golang wrote: > Unnecessary new line? http://codereview.appspot.com/183111/diff/3/1002#newcode491 src/pkg/xgb/xgb.go:491: err = os.NewError("searching Xauthority file: unknown home directory.") Fixed. On 2010/01/05 12:56:21, nigeltao_golang wrote: > I'd just write "Xauthority not found: unknown HOME"
Sign in to reply to this message.
http://codereview.appspot.com/183111/diff/6/1009 File src/pkg/xgb/auth.go (right): http://codereview.appspot.com/183111/diff/6/1009#newcode16 src/pkg/xgb/auth.go:16: b := make([]byte, 256, 256) Just say var b [256]byte. http://codereview.appspot.com/183111/diff/6/1009#newcode39 src/pkg/xgb/auth.go:39: } else { No need for an else, just take the "defer r.Close()" out of the {}. http://codereview.appspot.com/183111/diff/6/1009#newcode42 src/pkg/xgb/auth.go:42: I'd wrap r in a bufio.NewReader, as per exp/draw/x11/auth.go. http://codereview.appspot.com/183111/diff/6/1010 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/6/1010#newcode96 src/pkg/xgb/xgb.go:96: func get16BE(buf []byte) uint16 { get16BE and getString should also move to auth.go. http://codereview.appspot.com/183111/diff/6/1010#newcode307 src/pkg/xgb/xgb.go:307: // If the display string is empty it will be taken from os.Getenv("DISPLAY"). s/the display string/display/ http://codereview.appspot.com/183111/diff/6/1010#newcode341 src/pkg/xgb/xgb.go:341: if len(authName) != 18 || len(authData) != 16 { I'm not sure if this works for Tor's system. Tor, any thoughts? http://codereview.appspot.com/183111/diff/6/1010#newcode389 src/pkg/xgb/xgb.go:389: fmt.Println("error parsing screen number, using 0 as the default screen") A library shouldn't print to stdout. If the DISPLAY is malformed, just return the error. http://codereview.appspot.com/183111/diff/6/1010#newcode431 src/pkg/xgb/xgb.go:431: func parseDisplay(dispStr string) (protocol, host, display, screen string, err os.Error) { On second thoughts, rather than returning all these different strings, how about calling this function connect(display string) and returning (net.Conn, os.Error)?
Sign in to reply to this message.
It generally looks good to me. Let me ping Tor... http://codereview.appspot.com/183111/diff/1014/1015 File src/pkg/xgb/auth.go (right): http://codereview.appspot.com/183111/diff/1014/1015#newcode41 src/pkg/xgb/auth.go:41: const familyLocal = 256 I'd add the // As per /usr/include/X11/Xauth.h comment here, otherwise this 256 looks like it's related to the previous line, when that's really just a coincidence. http://codereview.appspot.com/183111/diff/1014/1016 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/1014/1016#newcode289 src/pkg/xgb/xgb.go:289: // If the display is empty it will be taken from os.Getenv("DISPLAY"). s/the display/display/ http://codereview.appspot.com/183111/diff/1014/1016#newcode415 src/pkg/xgb/xgb.go:415: dotPos := strings.LastIndex(display, ".") s/dotPos/dotIdx/ would be more consistent with your other variable names.
Sign in to reply to this message.
PTAL http://codereview.appspot.com/183111/diff/1014/1015 File src/pkg/xgb/auth.go (right): http://codereview.appspot.com/183111/diff/1014/1015#newcode41 src/pkg/xgb/auth.go:41: const familyLocal = 256 Fixed. On 2010/01/10 02:26:15, nigeltao_golang wrote: > I'd add the // As per /usr/include/X11/Xauth.h comment here, otherwise this 256 > looks like it's related to the previous line, when that's really just a > coincidence. http://codereview.appspot.com/183111/diff/1014/1016 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/1014/1016#newcode289 src/pkg/xgb/xgb.go:289: // If the display is empty it will be taken from os.Getenv("DISPLAY"). Fixed. On 2010/01/10 02:26:15, nigeltao_golang wrote: > s/the display/display/ http://codereview.appspot.com/183111/diff/1014/1016#newcode415 src/pkg/xgb/xgb.go:415: dotPos := strings.LastIndex(display, ".") Fixed. On 2010/01/10 02:26:15, nigeltao_golang wrote: > s/dotPos/dotIdx/ would be more consistent with your other variable names.
Sign in to reply to this message.
Russ -- can you give this a quick look?
Sign in to reply to this message.
looks pretty good. a bunch of small nits. http://codereview.appspot.com/183111/diff/3002/2003 File src/pkg/xgb/auth.go (right): http://codereview.appspot.com/183111/diff/3002/2003#newcode21 src/pkg/xgb/auth.go:21: func getString(r io.Reader, b []byte) (s string, err os.Error) { Split into getBytes, which returns []byte, and then getString, which wraps getBytes to return string. http://codereview.appspot.com/183111/diff/3002/2003#newcode24 src/pkg/xgb/auth.go:24: return It's good to be explicit on returns like this return "", err http://codereview.appspot.com/183111/diff/3002/2003#newcode27 src/pkg/xgb/auth.go:27: return s, os.NewError("Entry too long for buffer") error text should begin lowercase: s/Entry/string/ also change s to "". http://codereview.appspot.com/183111/diff/3002/2003#newcode31 src/pkg/xgb/auth.go:31: return return "", err http://codereview.appspot.com/183111/diff/3002/2003#newcode36 src/pkg/xgb/auth.go:36: // Reads the ~/.Xauthority file and returns the name/data pair for the DISPLAY. comments should start with nouns // readAuthority reads the ... for the DISPLAY. // If hostname == "" or hostname == "localhost", // readAuthority uses the system's host name (as returned by os.Hostname) instead. http://codereview.appspot.com/183111/diff/3002/2003#newcode37 src/pkg/xgb/auth.go:37: func readAuthority(hostname, display string) (name, data string, err os.Error) { should return name string, data []byte since data is not text. http://codereview.appspot.com/183111/diff/3002/2003#newcode38 src/pkg/xgb/auth.go:38: // b is a scratch buffer to use, and should be at least 256 bytes long s/,// http://codereview.appspot.com/183111/diff/3002/2003#newcode40 src/pkg/xgb/auth.go:40: var b [256]byte Is 256 a known limit? Should it be bigger? http://codereview.appspot.com/183111/diff/3002/2003#newcode48 src/pkg/xgb/auth.go:48: return return "", "", err http://codereview.appspot.com/183111/diff/3002/2003#newcode56 src/pkg/xgb/auth.go:56: err = os.NewError("Xauthority not found: unknown HOME") s/unknown HOME/$XAUTHORITY, $HOME not set http://codereview.appspot.com/183111/diff/3002/2003#newcode57 src/pkg/xgb/auth.go:57: return return "", "", err http://codereview.appspot.com/183111/diff/3002/2003#newcode64 src/pkg/xgb/auth.go:64: return return "", "", err http://codereview.appspot.com/183111/diff/3002/2003#newcode69 src/pkg/xgb/auth.go:69: for err == nil { Looks like this is the same as for { since the inner loop doesn't edit err http://codereview.appspot.com/183111/diff/3002/2003#newcode72 src/pkg/xgb/auth.go:72: return the err := makes a new err because you are not at top level. so the return value named err has not been modified. this is why it's a good idea to use explicit returns in complicated functions return "", "", err same comment on all the returns below http://codereview.appspot.com/183111/diff/3002/2003#newcode90 src/pkg/xgb/auth.go:90: data0, err := getString(br, b[0:]) s/getString/getBytes/ http://codereview.appspot.com/183111/diff/3002/2004 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/3002/2004#newcode303 src/pkg/xgb/xgb.go:303: if len(authName) != 18 || len(authData) != 16 { || authName != "MIT-MAGIC-COOKIE-1" ! (what if it's MIT-MAGIC-COOKIE-2?) http://codereview.appspot.com/183111/diff/3002/2004#newcode304 src/pkg/xgb/xgb.go:304: return nil, os.NewError("Unsupported Xauth") lower case, and add detail: "unsupported auth protocol " + authName http://codereview.appspot.com/183111/diff/3002/2004#newcode387 src/pkg/xgb/xgb.go:387: var protocol, scr string move these closer to their uses http://codereview.appspot.com/183111/diff/3002/2004#newcode393 src/pkg/xgb/xgb.go:393: add display0 := display http://codereview.appspot.com/183111/diff/3002/2004#newcode395 src/pkg/xgb/xgb.go:395: return nil, os.NewError("bad display string") s/bad/empty/ http://codereview.appspot.com/183111/diff/3002/2004#newcode398 src/pkg/xgb/xgb.go:398: slashIdx := strings.LastIndex(display, "/") before this line: var protocol string http://codereview.appspot.com/183111/diff/3002/2004#newcode399 src/pkg/xgb/xgb.go:399: if slashIdx > -1 { more idiomatic: >= 0 http://codereview.appspot.com/183111/diff/3002/2004#newcode405 src/pkg/xgb/xgb.go:405: if colonIdx == -1 { < 0 http://codereview.appspot.com/183111/diff/3002/2004#newcode406 src/pkg/xgb/xgb.go:406: return nil, os.NewError("bad display string") "bad display string: " + display0 http://codereview.appspot.com/183111/diff/3002/2004#newcode412 src/pkg/xgb/xgb.go:412: return nil, os.NewError("bad display string") "bad display string: " + display0 http://codereview.appspot.com/183111/diff/3002/2004#newcode415 src/pkg/xgb/xgb.go:415: dotIdx := strings.LastIndex(display, ".") before this line: var scr string http://codereview.appspot.com/183111/diff/3002/2004#newcode416 src/pkg/xgb/xgb.go:416: if dotIdx == -1 { < 0 http://codereview.appspot.com/183111/diff/3002/2004#newcode425 src/pkg/xgb/xgb.go:425: return nil, os.NewError("bad display string: " + err.String()) s/err.String()/display0 http://codereview.appspot.com/183111/diff/3002/2004#newcode431 src/pkg/xgb/xgb.go:431: return nil, os.NewError("bad display string: " + err.String()) s/err.String()/display0/ http://codereview.appspot.com/183111/diff/3002/2004#newcode439 src/pkg/xgb/xgb.go:439: return nil, os.NewError("cannot connect: " + err.String()) "cannot connect to " + display0 + ": " + err.String() http://codereview.appspot.com/183111/diff/3002/2004#newcode444 src/pkg/xgb/xgb.go:444: return nil, os.NewError("cannot connect: " + err.String()) same
Sign in to reply to this message.
Hello nigeltao_golang, rsc (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/183111/diff/3002/2003 File src/pkg/xgb/auth.go (right): http://codereview.appspot.com/183111/diff/3002/2003#newcode21 src/pkg/xgb/auth.go:21: func getString(r io.Reader, b []byte) (s string, err os.Error) { Fixed. On 2010/01/12 17:57:23, rsc wrote: > Split into getBytes, which returns []byte, > and then getString, which wraps getBytes to return string. > http://codereview.appspot.com/183111/diff/3002/2003#newcode24 src/pkg/xgb/auth.go:24: return Fixed. On 2010/01/12 17:57:23, rsc wrote: > It's good to be explicit on returns like this > > return "", err > http://codereview.appspot.com/183111/diff/3002/2003#newcode27 src/pkg/xgb/auth.go:27: return s, os.NewError("Entry too long for buffer") Fixed. On 2010/01/12 17:57:23, rsc wrote: > error text should begin lowercase: s/Entry/string/ > also change s to "". > http://codereview.appspot.com/183111/diff/3002/2003#newcode31 src/pkg/xgb/auth.go:31: return Fixed. On 2010/01/12 17:57:23, rsc wrote: > return "", err > http://codereview.appspot.com/183111/diff/3002/2003#newcode36 src/pkg/xgb/auth.go:36: // Reads the ~/.Xauthority file and returns the name/data pair for the DISPLAY. Fixed. On 2010/01/12 17:57:23, rsc wrote: > comments should start with nouns > > // readAuthority reads the ... for the DISPLAY. > // If hostname == "" or hostname == "localhost", > // readAuthority uses the system's host name (as returned by os.Hostname) > instead. > http://codereview.appspot.com/183111/diff/3002/2003#newcode37 src/pkg/xgb/auth.go:37: func readAuthority(hostname, display string) (name, data string, err os.Error) { Fixed. On 2010/01/12 17:57:23, rsc wrote: > should return name string, data []byte > since data is not text. > http://codereview.appspot.com/183111/diff/3002/2003#newcode38 src/pkg/xgb/auth.go:38: // b is a scratch buffer to use, and should be at least 256 bytes long Fixed. On 2010/01/12 17:57:23, rsc wrote: > s/,// > http://codereview.appspot.com/183111/diff/3002/2003#newcode40 src/pkg/xgb/auth.go:40: var b [256]byte Yes. You can check at http://refspecs.freestandards.org/LSB_2.0.1/LSB-Core/LSB-Core.html On 2010/01/12 17:57:23, rsc wrote: > Is 256 a known limit? Should it be bigger? > http://codereview.appspot.com/183111/diff/3002/2003#newcode48 src/pkg/xgb/auth.go:48: return Fixed. On 2010/01/12 17:57:23, rsc wrote: > return "", "", err > http://codereview.appspot.com/183111/diff/3002/2003#newcode56 src/pkg/xgb/auth.go:56: err = os.NewError("Xauthority not found: unknown HOME") Fixed. On 2010/01/12 17:57:23, rsc wrote: > s/unknown HOME/$XAUTHORITY, $HOME not set > http://codereview.appspot.com/183111/diff/3002/2003#newcode57 src/pkg/xgb/auth.go:57: return Fixed. On 2010/01/12 17:57:23, rsc wrote: > return "", "", err > http://codereview.appspot.com/183111/diff/3002/2003#newcode64 src/pkg/xgb/auth.go:64: return Fixed. On 2010/01/12 17:57:23, rsc wrote: > return "", "", err > http://codereview.appspot.com/183111/diff/3002/2003#newcode69 src/pkg/xgb/auth.go:69: for err == nil { Fixed. On 2010/01/12 17:57:23, rsc wrote: > Looks like this is the same as > > for { > > since the inner loop doesn't edit err http://codereview.appspot.com/183111/diff/3002/2003#newcode72 src/pkg/xgb/auth.go:72: return Fixed. On 2010/01/12 17:57:23, rsc wrote: > the err := makes a new err because you are not at top level. > so the return value named err has not been modified. > this is why it's a good idea to use explicit returns in > complicated functions > > return "", "", err > > same comment on all the returns below > http://codereview.appspot.com/183111/diff/3002/2003#newcode90 src/pkg/xgb/auth.go:90: data0, err := getString(br, b[0:]) Fixed. On 2010/01/12 17:57:23, rsc wrote: > s/getString/getBytes/ > http://codereview.appspot.com/183111/diff/3002/2004 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/3002/2004#newcode303 src/pkg/xgb/xgb.go:303: if len(authName) != 18 || len(authData) != 16 { I don't know. I got this code from exp/draw/x11/conn.go. Should this code be removed? On 2010/01/12 17:57:23, rsc wrote: > || authName != "MIT-MAGIC-COOKIE-1" ! > > (what if it's MIT-MAGIC-COOKIE-2?) > http://codereview.appspot.com/183111/diff/3002/2004#newcode304 src/pkg/xgb/xgb.go:304: return nil, os.NewError("Unsupported Xauth") Fixed. On 2010/01/12 17:57:23, rsc wrote: > lower case, and add detail: > "unsupported auth protocol " + authName > http://codereview.appspot.com/183111/diff/3002/2004#newcode387 src/pkg/xgb/xgb.go:387: var protocol, scr string Fixed. On 2010/01/12 17:57:23, rsc wrote: > move these closer to their uses > http://codereview.appspot.com/183111/diff/3002/2004#newcode393 src/pkg/xgb/xgb.go:393: Fixed. On 2010/01/12 17:57:23, rsc wrote: > add > > display0 := display > http://codereview.appspot.com/183111/diff/3002/2004#newcode395 src/pkg/xgb/xgb.go:395: return nil, os.NewError("bad display string") Fixed. On 2010/01/12 17:57:23, rsc wrote: > s/bad/empty/ > http://codereview.appspot.com/183111/diff/3002/2004#newcode398 src/pkg/xgb/xgb.go:398: slashIdx := strings.LastIndex(display, "/") Fixed. On 2010/01/12 17:57:23, rsc wrote: > before this line: var protocol string > http://codereview.appspot.com/183111/diff/3002/2004#newcode399 src/pkg/xgb/xgb.go:399: if slashIdx > -1 { Fixed. On 2010/01/12 17:57:23, rsc wrote: > more idiomatic: >= 0 > http://codereview.appspot.com/183111/diff/3002/2004#newcode405 src/pkg/xgb/xgb.go:405: if colonIdx == -1 { Fixed. On 2010/01/12 17:57:23, rsc wrote: > < 0 > http://codereview.appspot.com/183111/diff/3002/2004#newcode406 src/pkg/xgb/xgb.go:406: return nil, os.NewError("bad display string") Fixed. On 2010/01/12 17:57:23, rsc wrote: > "bad display string: " + display0 > http://codereview.appspot.com/183111/diff/3002/2004#newcode412 src/pkg/xgb/xgb.go:412: return nil, os.NewError("bad display string") Fixed. On 2010/01/12 17:57:23, rsc wrote: > "bad display string: " + display0 > http://codereview.appspot.com/183111/diff/3002/2004#newcode416 src/pkg/xgb/xgb.go:416: if dotIdx == -1 { Fixed. On 2010/01/12 17:57:23, rsc wrote: > < 0 > http://codereview.appspot.com/183111/diff/3002/2004#newcode425 src/pkg/xgb/xgb.go:425: return nil, os.NewError("bad display string: " + err.String()) Fixed. On 2010/01/12 17:57:23, rsc wrote: > s/err.String()/display0 > http://codereview.appspot.com/183111/diff/3002/2004#newcode431 src/pkg/xgb/xgb.go:431: return nil, os.NewError("bad display string: " + err.String()) Fixed. On 2010/01/12 17:57:23, rsc wrote: > s/err.String()/display0/ > http://codereview.appspot.com/183111/diff/3002/2004#newcode439 src/pkg/xgb/xgb.go:439: return nil, os.NewError("cannot connect: " + err.String()) Fixed. On 2010/01/12 17:57:23, rsc wrote: > "cannot connect to " + display0 + ": " + err.String() > http://codereview.appspot.com/183111/diff/3002/2004#newcode444 src/pkg/xgb/xgb.go:444: return nil, os.NewError("cannot connect: " + err.String()) Fixed. On 2010/01/12 17:57:23, rsc wrote: > same >
Sign in to reply to this message.
On OS X you can also install an X11 server as well. But my $DISPLAY variable says "/tmp/launch-2iR8Bm/:0" which is the file I have to used instead of "/tmp/.X11-unix/X"+c.display . Would you be able to make the code work for the weird OS X setup work?
Sign in to reply to this message.
This is just a workaround before xgb add support for OS X.
First make sure your X server is listening on TCP connection.
Then in your code, you should call the xgb.Dial like this:
xgb.Dial("tcp/yourhostname:0")
On 2010/01/15 14:59:00, jhh wrote:
> On OS X you can also install an X11 server as well. But my $DISPLAY variable
> says "/tmp/launch-2iR8Bm/:0" which is the file I have to used instead of
> "/tmp/.X11-unix/X"+c.display . Would you be able to make the code work for the
> weird OS X setup work?
Sign in to reply to this message.
LGTM please fix one more nit http://codereview.appspot.com/183111/diff/4001/2008 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/4001/2008#newcode303 src/pkg/xgb/xgb.go:303: if authName != "MIT-MAGIC-COOKIE-1" || len(authName) != 18 || len(authData) != 16 { can drop len(authName) != 18
Sign in to reply to this message.
Hello nigeltao_golang, rsc, jhh (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/183111/diff/4001/2008 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/4001/2008#newcode303 src/pkg/xgb/xgb.go:303: if authName != "MIT-MAGIC-COOKIE-1" || len(authName) != 18 || len(authData) != 16 { Fixed. On 2010/01/15 20:58:00, rsc wrote: > can drop len(authName) != 18 >
Sign in to reply to this message.
http://codereview.appspot.com/183111/diff/4003/2017 File src/pkg/xgb/auth.go (right): http://codereview.appspot.com/183111/diff/4003/2017#newcode13 src/pkg/xgb/auth.go:13: func get16BE(r io.Reader, b []byte) (uint16, os.Error) { I'd add a 'U', so it's called getU16BE. http://codereview.appspot.com/183111/diff/4003/2017#newcode45 src/pkg/xgb/auth.go:45: // if hostname == "" or hostname == "localhost", The "if hostname ..." comment should be a full sentence, and hence start with a capital-I and end with a full stop.
Sign in to reply to this message.
Hello nigeltao_golang, rsc, jhh (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/183111/diff/4003/2017 File src/pkg/xgb/auth.go (right): http://codereview.appspot.com/183111/diff/4003/2017#newcode13 src/pkg/xgb/auth.go:13: func get16BE(r io.Reader, b []byte) (uint16, os.Error) { Fixed. On 2010/01/18 13:15:22, nigeltao_golang wrote: > I'd add a 'U', so it's called getU16BE. http://codereview.appspot.com/183111/diff/4003/2017#newcode45 src/pkg/xgb/auth.go:45: // if hostname == "" or hostname == "localhost", Fixed. On 2010/01/18 13:15:22, nigeltao_golang wrote: > The "if hostname ..." comment should be a full sentence, and hence start with a > capital-I and end with a full stop.
Sign in to reply to this message.
Added code for checking weird $DISPLAY variable on OSX. jhh... please test this code coz I don't have any OSX.
Sign in to reply to this message.
_, err := xgb.Dial("")
works fine for me (XServer starts up and err is == nil).
The current version in the repos shows the same behavior with
_, err := xgb.Dial(os.Getenv("DISPLAY"))
Sign in to reply to this message.
Yes. The current version in the repos is working fine for OS X, but not for
Linux (without X server listening to TCP connection).
On 2010/01/18 23:26:37, jhh wrote:
> _, err := xgb.Dial("")
> works fine for me (XServer starts up and err is == nil).
>
> The current version in the repos shows the same behavior with
> _, err := xgb.Dial(os.Getenv("DISPLAY"))
Sign in to reply to this message.
I'm not sure that the connect function handles all the various forms of $DISPLAY on Linux and Mac, unix and tcp sockets. Still, I'm not sure if one person can test all the different formats. I'm tempted to just check the thing in, and then deal with any bug reports as they arise. As I said on http://codereview.appspot.com/186130, I think it might be useful to add some examples of the various $DISPLAY formats to the doc comment to connect. http://codereview.appspot.com/183111/diff/4013/4015 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/4013/4015#newcode411 src/pkg/xgb/xgb.go:411: c.host = display[0:colonIdx] Is 0 the correct value for the slice boundary? If slashIdx >= 0, then does the "host" incorrectly start with the protocol? Is slashIdx+1 the right thing here?
Sign in to reply to this message.
Also, Russ may know more about the process than I do, but if this is your first contribution to the Go source tree, then I think you have to complete a CLA: http://golang.org/doc/contribute.html#copyright
Sign in to reply to this message.
I've already filled the CLA form. Please take another look. http://codereview.appspot.com/183111/diff/4013/4015 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/4013/4015#newcode411 src/pkg/xgb/xgb.go:411: c.host = display[0:colonIdx] Oops, my mistake. Fixed. On 2010/01/19 13:16:03, nigeltao_golang wrote: > Is 0 the correct value for the slice boundary? If slashIdx >= 0, then does the > "host" incorrectly start with the protocol? Is slashIdx+1 the right thing here? http://codereview.appspot.com/183111/diff/2027/2029#newcode291 src/pkg/xgb/xgb.go:291: c, err := connect(display) I added the examples here so anyone who read the package documentation can read it too. What do you think?
Sign in to reply to this message.
http://codereview.appspot.com/183111/diff/2027/2029 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/2027/2029#newcode291 src/pkg/xgb/xgb.go:291: // Examples: On 2010/01/19 16:02:24, frm wrote: > I added the examples here so anyone who read the package documentation can read > it too. What do you think? For your third example, if a hostname is specified, then I think we want a TCP socket, not a Unix domain socket. For examples 3 and 4, I think the laddr (the 2nd argument to net.Dial) is "" not "hostname". Stylistically, I'd use only one tab rather than two. Also, delete the space character on line 292, position 3. http://codereview.appspot.com/183111/diff/2027/2029#newcode460 src/pkg/xgb/xgb.go:460: c.conn, err = net.Dial("unix", c.host, "/tmp/.X11-unix/X"+c.display) I don't think the laddr parameter should be c.host, just simply "".
Sign in to reply to this message.
Please take another look. http://codereview.appspot.com/183111/diff/2027/2029 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/2027/2029#newcode291 src/pkg/xgb/xgb.go:291: // Examples: Yes, you're right. Fixed the comment and the code. On 2010/01/19 23:10:50, nigeltao_golang wrote: > On 2010/01/19 16:02:24, frm wrote: > > I added the examples here so anyone who read the package documentation can > read > > it too. What do you think? > > For your third example, if a hostname is specified, then I think we want a TCP > socket, not a Unix domain socket. > > For examples 3 and 4, I think the laddr (the 2nd argument to net.Dial) is "" not > "hostname". > > Stylistically, I'd use only one tab rather than two. Also, delete the space > character on line 292, position 3. http://codereview.appspot.com/183111/diff/2027/2029#newcode460 src/pkg/xgb/xgb.go:460: c.conn, err = net.Dial("unix", c.host, "/tmp/.X11-unix/X"+c.display) Fixed. On 2010/01/19 23:10:50, nigeltao_golang wrote: > I don't think the laddr parameter should be c.host, just simply "".
Sign in to reply to this message.
Looks Good To Me. There's just one tiny thing to fix. http://codereview.appspot.com/183111/diff/2034/5007 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/2034/5007#newcode452 src/pkg/xgb/xgb.go:452: if len(protocol) != 0 { I'd phrase this section (lines 452-456) as: if protocol == "" { protocol = "tcp" } c.conn, err = net.Dial(protocol, "", c.host+":"+strconv.Uitoa(6000+dispnum))
Sign in to reply to this message.
PTAL. http://codereview.appspot.com/183111/diff/2034/5007 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/183111/diff/2034/5007#newcode452 src/pkg/xgb/xgb.go:452: if len(protocol) != 0 { Fixed. On 2010/01/20 23:26:07, nigeltao_golang wrote: > I'd phrase this section (lines 452-456) as: > if protocol == "" { > protocol = "tcp" > } > c.conn, err = net.Dial(protocol, "", c.host+":"+strconv.Uitoa(6000+dispnum))
Sign in to reply to this message.
LGTM. Russ -- what's the process for landing this? Does frm.adiputra have to make a separate AUTHORS/CONTRIBUTORS patch, a la revision 659417f8f0: http://code.google.com/p/go/source/detail?spec=svndfc46fd3467972bb33c49c3c812...
Sign in to reply to this message.
On 21/01/2010, at 7:01 PM, Nigel Tao wrote: > LGTM. > > Russ -- what's the process for landing this? Does frm.adiputra have to > make a separate AUTHORS/CONTRIBUTORS patch, a la revision 659417f8f0: > http://code.google.com/p/go/source/detail?spec=svndfc46fd3467972bb33c49c3c812... see the contribute.html page (i think). they need to put their name on the form, you need to check their name is on the form, and then they need to go into AUTHORS and CONTRIBUTORS. all of this need only happen once per contributor. -rob
Sign in to reply to this message.
It's easier if someone with direct commit access creates the A+C change, so the process is now that new contributors simply complete the CLA and then tell us when they have done so. We double-check that they're on the CLA list at Google and then create the A+C change, get it reviewed, and submit it. Then the original patch can be submitted. As Rob said, this only needs to happen once. Russ
Sign in to reply to this message.
I've completed the CLA. On 2010/01/21 15:38:47, rsc wrote: > It's easier if someone with direct commit access > creates the A+C change, so the process is now that > new contributors simply complete the CLA and then tell > us when they have done so. We double-check that they're > on the CLA list at Google and then create the A+C change, > get it reviewed, and submit it. > > Then the original patch can be submitted. As Rob said, > this only needs to happen once. > > Russ >
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=e45a594192d4 *** Add authentication. Other code fixing: - Fixed bugs in get32. - Fix code for parsing display string (as a new function). - Fix code for connecting to X server. The old code only work if the server is listening to TCP port, otherwise it doesn't work (at least in my PC). R=nigeltao_golang, rsc, jhh CC=golang-dev http://codereview.appspot.com/183111 Committer: Nigel Tao <nigeltao@golang.org>
Sign in to reply to this message.
On 2010/01/22 06:55:49, nigeltao_golang wrote: > *** Submitted as http://code.google.com/p/go/source/detail?r=e45a594192d4 *** > > Add authentication. > > Other code fixing: > - Fixed bugs in get32. > - Fix code for parsing display string (as a new function). > - Fix code for connecting to X server. The old code only work > if the server is listening to TCP port, otherwise it doesn't > work (at least in my PC). > > R=nigeltao_golang, rsc, jhh > CC=golang-dev > http://codereview.appspot.com/183111 > > Committer: Nigel Tao <mailto:nigeltao@golang.org> I think you should add auth.go to Makefile
Sign in to reply to this message.
2010/1/23 <surya@c77.in>: > I think you should add auth.go to Makefile Thanks to Ian for fixing this. http://code.google.com/p/go/source/detail?r=0957364f254f96b781ba6f88f36cdb9bb...
Sign in to reply to this message.
|
