code review 7398054: net, os, syscall: Plan 9: adjust error handling

net, os, syscall: Plan 9: adjust error handling

syscall: Use NewError for all system errors and introduce
        some new errors for compatibility with other packages
        and proper error handling in net. Also introduce
        Temporary and Timeout methods on ErrorString.

net: Make errors from dial, accept, listen functions follow the
        OpError standard and discern whether the underlying
        error came from syscall. Since Plan 9 uses a correspondence
        between file and network operations, all system error
        reporting happens through the underlying file operation.
        In Go code, we go through package os for file operations,
        so there is another level of indirection in error types.
        This change allows us to compare the errors with those in
        package syscall, when appropriate.

os: Just use the error string already present in package os,
        instead of calling out to package syscall.

[akumar, 2013-02-27 06:37:31]

Hello rsc@golang.org, ality@pbrane.org, rminnich@gmail.com, bradfitz@golang.org
(cc: golang-dev@googlegroups.com),

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

[bradfitz, 2013-02-27 06:55:49]

LGTM

On Tue, Feb 26, 2013 at 10:37 PM, <seed@mail.nanosouffle.net> wrote:

> Reviewers: rsc, ality, rminnich, bradfitz,
>
> Message:
> Hello rsc@golang.org, ality@pbrane.org, rminnich@gmail.com,
> bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net, os, syscall: Plan 9: adjust error handling
>
> syscall: Use NewError for all system errors and introduce
>         some new errors for compatibility with other packages
>         and proper error handling in net. Also introduce
>         Temporary and Timeout methods on ErrorString.
>
> net: Make errors from dial, accept, listen functions follow the
>         OpError standard.
>
> os: Just use the error string already present in package os,
>         instead of calling out to package syscall.
>
> Please review this at
https://codereview.appspot.**com/7398054/<https://codereview.appspot.com/7398...
>
> Affected files:
>   M src/pkg/net/ipsock_plan9.go
>   M src/pkg/os/exec/lp_plan9.go
>   M src/pkg/syscall/syscall_plan9.**go
>   M src/pkg/syscall/zerrors_plan9_**386.go
>   M src/pkg/syscall/zerrors_plan9_**amd64.go
>
>
> Index: src/pkg/net/ipsock_plan9.go
> ==============================**==============================**=======
> --- a/src/pkg/net/ipsock_plan9.go
> +++ b/src/pkg/net/ipsock_plan9.go
> @@ -107,29 +107,23 @@
>  func dialPlan9(net string, laddr, raddr Addr) (*netFD, error) {
>         f, dest, proto, name, err := startPlan9(net, raddr)
>         if err != nil {
> -               return nil, err
> +               return nil, &OpError{"dial", net, raddr, err}
>         }
>         _, err = f.WriteString("connect " + dest)
>         if err != nil {
>                 f.Close()
> -               return nil, err
> +               return nil, &OpError{"dial", f.Name(), raddr, err}
>         }
>         data, err := os.OpenFile("/net/"+proto+"/"+**name+"/data",
> os.O_RDWR, 0)
>         if err != nil {
>                 f.Close()
> -               return nil, err
> +               return nil, &OpError{"dial", data.Name(), raddr, err}
>         }
>         laddr, err = readPlan9Addr(proto, "/net/"+proto+"/"+name+"/**
> local")
>         if err != nil {
>                 data.Close()
>                 f.Close()
> -               return nil, err
> -       }
> -       raddr, err = readPlan9Addr(proto, "/net/"+proto+"/"+name+"/**
> remote")
> -       if err != nil {
> -               data.Close()
> -               f.Close()
> -               return nil, err
> +               return nil, &OpError{"dial", proto, raddr, err}
>         }
>         return newFD(proto, name, f, data, laddr, raddr), nil
>  }
> @@ -137,17 +131,17 @@
>  func listenPlan9(net string, laddr Addr) (*netFD, error) {
>         f, dest, proto, name, err := startPlan9(net, laddr)
>         if err != nil {
> -               return nil, err
> +               return nil, &OpError{"listen", net, laddr, err}
>         }
>         _, err = f.WriteString("announce " + dest)
>         if err != nil {
>                 f.Close()
> -               return nil, err
> +               return nil, &OpError{"announce", proto, laddr, err}
>         }
>         laddr, err = readPlan9Addr(proto, "/net/"+proto+"/"+name+"/**
> local")
>         if err != nil {
>                 f.Close()
> -               return nil, err
> +               return nil, &OpError{Op: "listen", Net: net, Err: err}
>         }
>         return newFD(proto, name, f, nil, laddr, nil), nil
>  }
> @@ -159,25 +153,25 @@
>  func (l *netFD) acceptPlan9() (*netFD, error) {
>         f, err := os.Open(l.dir + "/listen")
>         if err != nil {
> -               return nil, err
> +               return nil, &OpError{"accept", l.dir + "/listen", l.laddr,
> err}
>         }
>         var buf [16]byte
>         n, err := f.Read(buf[:])
>         if err != nil {
>                 f.Close()
> -               return nil, err
> +               return nil, &OpError{"accept", l.dir + "/listen", l.laddr,
> err}
>         }
>         name := string(buf[:n])
>         data, err := os.OpenFile("/net/"+l.proto+"/**"+name+"/data",
> os.O_RDWR, 0)
>         if err != nil {
>                 f.Close()
> -               return nil, err
> +               return nil, &OpError{"accept", data.Name(), l.laddr, err}
>         }
>         raddr, err := readPlan9Addr(l.proto, "/net/"+l.proto+"/"+name+"/**
> remote")
>         if err != nil {
>                 data.Close()
>                 f.Close()
> -               return nil, err
> +               return nil, &OpError{"accept", l.proto, l.laddr, err}
>         }
>         return newFD(l.proto, name, f, data, l.laddr, raddr), nil
>  }
> Index: src/pkg/os/exec/lp_plan9.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec/lp_plan9.go
> +++ b/src/pkg/os/exec/lp_plan9.go
> @@ -8,7 +8,6 @@
>         "errors"
>         "os"
>         "strings"
> -       "syscall"
>  )
>
>  // ErrNotFound is the error resulting if a path search failed to find an
> executable file.
> @@ -22,7 +21,7 @@
>         if m := d.Mode(); !m.IsDir() && m&0111 != 0 {
>                 return nil
>         }
> -       return syscall.EPERM
> +       return os.ErrPermission
>  }
>
>  // LookPath searches for an executable binary named file
> Index: src/pkg/syscall/syscall_plan9.**go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_**plan9.go
> +++ b/src/pkg/syscall/syscall_**plan9.go
> @@ -23,6 +23,14 @@
>  // NewError converts s to an ErrorString, which satisfies the Error
> interface.
>  func NewError(s string) error { return ErrorString(s) }
>
> +func (e ErrorString) Temporary() bool {
> +       return e == EINTR || e == EMFILE || e.Timeout()
> +}
> +
> +func (e ErrorString) Timeout() bool {
> +       return e == EBUSY || e == ETIMEDOUT
> +}
> +
>  // A Note is a string describing a process note.
>  // It implements the os.Signal interface.
>  type Note string
> @@ -37,9 +45,6 @@
>         Stdin  = 0
>         Stdout = 1
>         Stderr = 2
> -
> -       EAFNOSUPPORT = NewError("address family not supported by protocol")
> -       EISDIR       = NewError("file is a directory")
>  )
>
>  // For testing: clients can set this flag to force
> Index: src/pkg/syscall/zerrors_plan9_**386.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/zerrors_**plan9_386.go
> +++ b/src/pkg/syscall/zerrors_**plan9_386.go
> @@ -28,12 +28,23 @@
>
>  // Errors
>  var (
> -       EINVAL       = errors.New("bad arg in system call")
> -       ENOTDIR      = errors.New("not a directory")
> -       ENOENT       = errors.New("file does not exist")
> -       EEXIST       = errors.New("file already exists")
> -       EIO          = errors.New("i/o error")
> -       ENAMETOOLONG = errors.New("file name too long")
> -       EPERM        = errors.New("permission denied")
> -       EPLAN9       = errors.New("not supported by plan 9")
> +       EINVAL       = NewError("bad arg in system call")
> +       ENOTDIR      = NewError("not a directory")
> +       EISDIR       = NewError("file is a directory")
> +       ENOENT       = NewError("file does not exist")
> +       EEXIST       = NewError("file already exists")
> +       EMFILE       = NewError("no free file descriptors")
> +       EIO          = NewError("i/o error")
> +       ENAMETOOLONG = NewError("file name too long")
> +       EINTR        = NewError("interrupted")
> +       EPERM        = NewError("permission denied")
> +       EBUSY        = NewError("no free devices")
> +       ETIMEDOUT    = NewError("connection timed out")
> +       EPLAN9       = NewError("not supported by plan 9")
> +
> +       // The following errors do not correspond to any
> +       // Plan 9 system messages. Invented to support
> +       // what package os and others expect.
> +       EACCES       = NewError("access permission denied")
> +       EAFNOSUPPORT = NewError("address family not supported by protocol")
>  )
> Index: src/pkg/syscall/zerrors_plan9_**amd64.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/zerrors_**plan9_amd64.go
> +++ b/src/pkg/syscall/zerrors_**plan9_amd64.go
> @@ -28,12 +28,23 @@
>
>  // Errors
>  var (
> -       EINVAL       = errors.New("bad arg in system call")
> -       ENOTDIR      = errors.New("not a directory")
> -       ENOENT       = errors.New("file does not exist")
> -       EEXIST       = errors.New("file already exists")
> -       EIO          = errors.New("i/o error")
> -       ENAMETOOLONG = errors.New("file name too long")
> -       EPERM        = errors.New("permission denied")
> -       EPLAN9       = errors.New("not supported by plan 9")
> +       EINVAL       = NewError("bad arg in system call")
> +       ENOTDIR      = NewError("not a directory")
> +       EISDIR       = NewError("file is a directory")
> +       ENOENT       = NewError("file does not exist")
> +       EEXIST       = NewError("file already exists")
> +       EMFILE       = NewError("no free file descriptors")
> +       EIO          = NewError("i/o error")
> +       ENAMETOOLONG = NewError("file name too long")
> +       EINTR        = NewError("interrupted")
> +       EPERM        = NewError("permission denied")
> +       EBUSY        = NewError("no free devices")
> +       ETIMEDOUT    = NewError("connection timed out")
> +       EPLAN9       = NewError("not supported by plan 9")
> +
> +       // The following errors do not correspond to any
> +       // Plan 9 system messages. Invented to support
> +       // what package os and others expect.
> +       EACCES       = NewError("access permission denied")
> +       EAFNOSUPPORT = NewError("address family not supported by protocol")
>  )
>
>
>

[ality, 2013-02-27 09:49:43]

The net and os changes look good but the
syscall changes are just too much for me.

I'm trying to get rid of all those errors
and you want to add more? Why not fix the
tests instead?

Obviously, we have different opinions on
this so we'll have to come to a consensus
somehow.

  Anthony

[akumar, 2013-02-27 14:41:45]

The error strings above the comments are
ones that are reported by the system. It
makes sense to check against these -- the
Temporary and Timeout methods are not
synthetic, as a simple test will show.

As for the strings below the comments,
the only new one is EACCES and the test
it allows us to build is doing a dir creation
test in `/', which can essentially return
an arbitrary error message to indicate
permission error (as it is dependent upon
the fs). The test itself is disabled for this
reason and the addition of this string is
a simple, non-aggressive way to get along
for now. Separating into POSIX vs Plan 9
didn't make sense for now, until the test
itself is changed.


On 27 February 2013 09:49, Anthony Martin <ality@pbrane.org> wrote:
> The net and os changes look good but the
> syscall changes are just too much for me.
>
> I'm trying to get rid of all those errors
> and you want to add more? Why not fix the
> tests instead?
>
> Obviously, we have different opinions on
> this so we'll have to come to a consensus
> somehow.
>
>   Anthony

[bradfitz, 2013-02-27 15:50:46]

On Wed, Feb 27, 2013 at 1:49 AM, Anthony Martin <ality@pbrane.org> wrote:

> The net and os changes look good but the
> syscall changes are just too much for me.
>
> I'm trying to get rid of all those errors
> and you want to add more? Why not fix the
> tests instead?
>
> Obviously, we have different opinions on
> this so we'll have to come to a consensus
> somehow.
>

Akshat, Anthony,

To be clear, I'm not picking sides here and have no plan9 knowledge.  My
LGTM was purely a quick sanity check for style & impact on the tree as a
whole.

Please always wait for a another plan9 person to LGTM also before
submitting.

[akumar, 2013-02-27 17:08:08]

PTAL.

Updated error handling in package net and removed the import of "errors" in
package syscall's z* files.

[bradfitz, 2013-02-27 23:24:20]

LGTM still

Leaving for Anthony to submit, once he's happy.

On Wed, Feb 27, 2013 at 9:08 AM, <seed@mail.nanosouffle.net> wrote:

> PTAL.
>
> Updated error handling in package net and removed the import of "errors"
> in package syscall's z* files.
>
>
>
https://codereview.appspot.**com/7398054/<https://codereview.appspot.com/7398...
>

[rminnich, 2013-02-28 05:42:38]

LGTM

I think any remaining disagreements can be resolved with another bit of
trimming, but actually I like this one as is.

[rminnich, 2013-02-28 05:43:25]

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

net, os, syscall: Plan 9: adjust error handling

syscall: Use NewError for all system errors and introduce
        some new errors for compatibility with other packages
        and proper error handling in net. Also introduce
        Temporary and Timeout methods on ErrorString.

net: Make errors from dial, accept, listen functions follow the
        OpError standard and discern whether the underlying
        error came from syscall. Since Plan 9 uses a correspondence
        between file and network operations, all system error
        reporting happens through the underlying file operation.
        In Go code, we go through package os for file operations,
        so there is another level of indirection in error types.
        This change allows us to compare the errors with those in
        package syscall, when appropriate.

os: Just use the error string already present in package os,
        instead of calling out to package syscall.

R=rsc, ality, rminnich, bradfitz
CC=golang-dev
https://codereview.appspot.com/7398054

Committer: Ron Minnich <rminnich@gmail.com>