syslog: restart connections automatically. Fix priority values.

syslog: restart connections automatically
  Implements deferred connections + single-attempt automatic
  retry should the connection drop.

  Fixes issue 2264

syslog: fix priority values.
  hard define priority values and include facility values as
  defined in the RFC and in the C library.

[adg, 2011-09-20 06:53:02]

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode22
src/pkg/syslog/syslog.go:22: SeverityPresent = Priority(0x100)
If they're internal, then give them unexported names

severity
severityMask
facility
facilityMask

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode28
src/pkg/syslog/syslog.go:28: LOG_EMERG   = Priority(0) | SeverityPresent
Instead, you can do

const (
  LOG_EMERG = Priority(iota) | SeverityPresent
  LOG_ALERT
  LOG_CRIT
etc
)

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode37
src/pkg/syslog/syslog.go:37: LOG_KERN     = Priority(0<<3) | FacilityPresent
)

const (
  LOG_KERN = Priority(iota<<3) | facility
  LOG_USER
etc
)

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode114
src/pkg/syslog/syslog.go:114: log.Printf("writer.Reconnect() on old
connection.\n")
Seems like a superfluous log message.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode143
src/pkg/syslog/syslog.go:143: func (w *Writer) Write(b []byte) (bout int, err
os.Error) {
this logic is repeated in both functions.
instead, how about a third function which the others can wrap:

func (w *Writer) write(p Priority, v interface{}) (n int, err os.Error) {
   // reconnect logic
   switch v := v.(type) {
   case string:
      n, err = w.conn.writeString(p, w.prefix, v)
   case []byte:
      n, err = w.conn.writeBytes(p, w.prefix, v)
   default:
      panic("syslog: attempt to write unacceptable type")
   }
   // retry logic
}

func (w *Writer) writeString(p Priority, s string) (int, os.Error) {
   return w.write(p, s)
}

func (w *Writer) Write(b []byte) (int, os.Error) {
   return w.write(p, b)
}

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode151
src/pkg/syslog/syslog.go:151: retry:
Use a for loop instead of the goto.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode179
src/pkg/syslog/syslog.go:179: log.Printf("Hit!")
weird log message.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog_test.go
File src/pkg/syslog/syslog_test.go (right):

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog_test.go#ne...
src/pkg/syslog/syslog_test.go:118: t.Fatalf("s.Debug() = '%q', but wanted '%q'",
rcvd, expected)
s/but wanted/want/g

[kuroneko, 2011-09-21 01:17:42]

Thanks for the feedback.

A patchset has been uploaded with the fixes I agree with.  Discussion follows.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode22
src/pkg/syslog/syslog.go:22: SeverityPresent = Priority(0x100)
On 2011/09/20 06:53:02, adg wrote:
> If they're internal, then give them unexported names
> 
> severity
> severityMask
> facility
> facilityMask

Done.  Although the Masks might be useful externally, I'll worry about that if
anybody complains later.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode28
src/pkg/syslog/syslog.go:28: LOG_EMERG   = Priority(0) | SeverityPresent
On 2011/09/20 06:53:02, adg wrote:
> Instead, you can do
> 
> const (
>   LOG_EMERG = Priority(iota) | SeverityPresent
>   LOG_ALERT

Not keen on this as value assignment via iota means that the value becomes
position dependent in the code -- if these values were entirely internal, I'd
have no problem with this, but as they're specified in an external
specification, my personal style is to explicitly code their values so there can
be no confusion.

Particularly notable, the Facility set is discontinuous as a few of the valid
values aren't usually in the headers, so iota doesn't really work there unless I
put in the missing (less commonly used) intermediates.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode114
src/pkg/syslog/syslog.go:114: log.Printf("writer.Reconnect() on old
connection.\n")
On 2011/09/20 06:53:02, adg wrote:
> Seems like a superfluous log message.
Whoops - it is - that was part of my own debugging.
Removed.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode143
src/pkg/syslog/syslog.go:143: func (w *Writer) Write(b []byte) (bout int, err
os.Error) {
On 2011/09/20 06:53:02, adg wrote:
> this logic is repeated in both functions.
> instead, how about a third function which the others can wrap:

Presumably a cast from string to []byte is cheap, so I've gone with implementing
a single writeBytes function and wrapping to it, rather than going with a switch
on type.

http://codereview.appspot.com/5078042/diff/1/src/pkg/syslog/syslog.go#newcode179
src/pkg/syslog/syslog.go:179: log.Printf("Hit!")
On 2011/09/20 06:53:02, adg wrote:
> weird log message.
again, leftover debugging. :(

[r, 2011-09-21 17:11:08]

http://codereview.appspot.com/5078042/diff/5001/src/pkg/syslog/syslog.go
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/5078042/diff/5001/src/pkg/syslog/syslog.go#newc...
src/pkg/syslog/syslog.go:57: )
I understand your objections but you're fighting the language.

const (
  // These are internal ...
  severityPresent Priority = 0x100
  ...
)

const (
  // From the RFCs...
  LOG_EMERG Priority = iota | severityPresent
  LOG_ALERT
  ...
)

const (
  // More from the RFCs...
  LOG_KERN Priority = (iota << 3) | facilityPresent
  LOG_USER
  ...
)

Yes, it's harder to see the value of LOG_FTP, but that's not very important.
It's not that easy anyway - quick, what is the value of (11<<3) |
facilityPresent? A user still needs to work it out.

[adg, 2012-08-08 00:47:55]

What happened to this? Can we close it?

[minux1, 2012-08-08 08:01:51]

On 2012/08/08 00:47:55, adg wrote:
> What happened to this? Can we close it?
issue 2264 is still open

[jeff.allen, 2012-11-29 09:26:13]

As of 14987:a041b45cc418 the priority/facility/severity part of this CL is fixed
(a different way).

The part of this CL related to retries is not implemented, so Issue 2204 is
still unaddressed.

I will try to submit a new CL with a fix for 2204 in it.

[dave_cheney.net, 2012-11-29 09:35:50]

thanks

On Thursday, November 29, 2012, wrote:

> As of 14987:a041b45cc418 the priority/facility/severity part of this CL
> is fixed (a different way).
>
> The part of this CL related to retries is not implemented, so Issue 2204
> is still unaddressed.
>
> I will try to submit a new CL with a fix for 2204 in it.
>
>
>
https://codereview.appspot.**com/5078042/<https://codereview.appspot.com/5078...
>