code review 6946057: net/smtp: add optional Hello method

net/smtp: add optional Hello method

Add a Hello method that allows clients to set the server sent in the EHLO/HELO exchange; the default remains localhost.
Based on CL 5555045 by rsc.

Fixes issue 4219.

[rick, 2012-12-14 22:34:27]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2012-12-17 00:08:04]

LGTM

https://codereview.appspot.com/6946057/diff/4001/src/pkg/net/smtp/smtp.go
File src/pkg/net/smtp/smtp.go (right):

https://codereview.appspot.com/6946057/diff/4001/src/pkg/net/smtp/smtp.go#new...
src/pkg/net/smtp/smtp.go:85: return errors.New("Hello called after other SMTP
exchanges")
"smtp: Hello called after other methods"

[rick, 2012-12-17 01:16:30]

PTAL

[rsc, 2012-12-17 01:18:37]

LGTM

[rsc, 2012-12-17 01:19:29]

*** Submitted as https://code.google.com/p/go/source/detail?r=d68fadb4593f ***

net/smtp: add optional Hello method

Add a Hello method that allows clients to set the server sent in the EHLO/HELO
exchange; the default remains localhost.
Based on CL 5555045 by rsc.

Fixes issue 4219.

R=golang-dev, rsc
CC=golang-dev
https://codereview.appspot.com/6946057

Committer: Russ Cox <rsc@golang.org>

[dave_cheney.net, 2012-12-17 04:00:27]

Hi Rick,

Would you be able to look into this issue,
https://code.google.com/p/go/issues/detail?id=4559, which I think is
related to the tests that you added in this CL.

Cheers

Dave

On Mon, Dec 17, 2012 at 12:19 PM,  <rsc@golang.org> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=d68fadb4593f ***
>
>
> net/smtp: add optional Hello method
>
> Add a Hello method that allows clients to set the server sent in the
> EHLO/HELO exchange; the default remains localhost.
> Based on CL 5555045 by rsc.
>
> Fixes issue 4219.
>
> R=golang-dev, rsc
> CC=golang-dev
> https://codereview.appspot.com/6946057
>
> Committer: Russ Cox <rsc@golang.org>
>
>
> https://codereview.appspot.com/6946057/

[rsc, 2012-12-17 15:35:30]

I tried this:

diff -r d2c79fccab13 src/pkg/net/smtp/smtp_test.go
--- a/src/pkg/net/smtp/smtp_test.go Mon Dec 17 23:01:36 2012 +0800
+++ b/src/pkg/net/smtp/smtp_test.go Mon Dec 17 10:34:35 2012 -0500
@@ -385,7 +385,11 @@
  }
  defer l.Close()

+ doneWriting := make(chan bool, 1)
  go func(l net.Listener, data []string, w *bufio.Writer) {
+ defer func() {
+ doneWriting <- true
+ }()
  i := 0
  conn, err := l.Accept()
  if err != nil {
@@ -429,10 +433,11 @@
  t.Errorf("%v", err)
  }

+ <-doneWriting
  bcmdbuf.Flush()
  actualcmds := cmdbuf.String()
  if client != actualcmds {
- t.Errorf("Got:\n%s\nExpected:\n%s", actualcmds, client)
+ t.Errorf("Got:\n%q\nExpected:\n%q", actualcmds, client)
  }
 }

but now I get (from GOMAXPROCS=2 go test -race):

smtp_test.go:440: Got:
"EHLO localhost\r\nHELO localhost\r\nMAIL
FROM:<test@example.com>\r\nRCPT
TO:<other@example.com>\r\nDATA\r\nFrom: test@example.com\r\nTo:
other@example.com\r\nSubject: SendMail test\r\n\r\nSendMail is working
for me.\r\n.\r\nQUIT\r\n\r\n"
Expected:
"EHLO localhost\r\nHELO localhost\r\nMAIL
FROM:<test@example.com>\r\nRCPT
TO:<other@example.com>\r\nDATA\r\nFrom: test@example.com\r\nTo:
other@example.com\r\nSubject: SendMail test\r\n\r\nSendMail is working
for me.\r\n.\r\nQUIT\r\n"

The difference between the two strings is the doubled \r\n after QUIT
in the 'Got' string.

Russ

[rick, 2012-12-17 15:37:57]

I fixed that with this CL: https://codereview.appspot.com/6944057

On Monday, December 17, 2012 10:35:27 AM UTC-5, rsc wrote:
>
> I tried this: 
>
> diff -r d2c79fccab13 src/pkg/net/smtp/smtp_test.go 
> --- a/src/pkg/net/smtp/smtp_test.go Mon Dec 17 23:01:36 2012 +0800 
> +++ b/src/pkg/net/smtp/smtp_test.go Mon Dec 17 10:34:35 2012 -0500 
> @@ -385,7 +385,11 @@ 
>   } 
>   defer l.Close() 
>
> + doneWriting := make(chan bool, 1) 
>   go func(l net.Listener, data []string, w *bufio.Writer) { 
> + defer func() { 
> + doneWriting <- true 
> + }() 
>   i := 0 
>   conn, err := l.Accept() 
>   if err != nil { 
> @@ -429,10 +433,11 @@ 
>   t.Errorf("%v", err) 
>   } 
>
> + <-doneWriting 
>   bcmdbuf.Flush() 
>   actualcmds := cmdbuf.String() 
>   if client != actualcmds { 
> - t.Errorf("Got:\n%s\nExpected:\n%s", actualcmds, client) 
> + t.Errorf("Got:\n%q\nExpected:\n%q", actualcmds, client) 
>   } 
>  } 
>
> but now I get (from GOMAXPROCS=2 go test -race): 
>
> smtp_test.go:440: Got: 
> "EHLO localhost\r\nHELO localhost\r\nMAIL 
> FROM:<te...@example.com <javascript:>>\r\nRCPT 
> TO:<ot...@example.com <javascript:>>\r\nDATA\r\nFrom:
te...@example.com<javascript:>\r\nTo: 
>
> ot...@example.com <javascript:>\r\nSubject: SendMail test\r\n\r\nSendMail 
> is working 
> for me.\r\n.\r\nQUIT\r\n\r\n" 
> Expected: 
> "EHLO localhost\r\nHELO localhost\r\nMAIL 
> FROM:<te...@example.com <javascript:>>\r\nRCPT 
> TO:<ot...@example.com <javascript:>>\r\nDATA\r\nFrom:
te...@example.com<javascript:>\r\nTo: 
>
> ot...@example.com <javascript:>\r\nSubject: SendMail test\r\n\r\nSendMail 
> is working 
> for me.\r\n.\r\nQUIT\r\n" 
>
> The difference between the two strings is the doubled \r\n after QUIT 
> in the 'Got' string. 
>
> Russ 
>