code review 6015046: syscall: remove the Signal type on Plan 9

syscall: remove the Signal type on Plan 9

Instead use a new type, "Note", whose underlying
type is just a string.  This change allows us to
remove the exported os.Plan9Note type.

[ality, 2012-04-12 13:32:45]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-04-12 14:17:47]

godoc won't look good with this (the docs on the vars will be sorted away
in a different section with the Signal interface type).  You may just have
to duplicate the docs between exec_posix.go and exec_plan9.go.


On Thu, Apr 12, 2012 at 6:32 AM, <ality@pbrane.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> syscall: remove the Signal type on Plan 9
>
> Instead use a new type, "Note", whose underlying
> type is just a string.  This change allows us to
> remove the exported os.Plan9Note type.
>
> Please review this at
http://codereview.appspot.com/**6015046/<http://codereview.appspot.com/6015046/>
>
> Affected files:
>  M src/pkg/os/exec.go
>  M src/pkg/os/exec_plan9.go
>  M src/pkg/os/exec_posix.go
>  M src/pkg/syscall/syscall_plan9.**go
>  M src/pkg/syscall/zerrors_plan9_**386.go
>
>
> Index: src/pkg/os/exec.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec.go
> +++ b/src/pkg/os/exec.go
> @@ -49,19 +49,15 @@
>  // A Signal represents an operating system signal.
>  // The usual underlying implementation is operating system-dependent:
>  // on Unix it is syscall.Signal.
> +//
> +// The only signal values guaranteed to be present on all systems
> +// are Interrupt (send the process an interrupt) and Kill (force
> +// the process to exit).
>  type Signal interface {
>        String() string
>        Signal() // to distinguish from other Stringers
>  }
>
> -// The only signal values guaranteed to be present on all systems
> -// are Interrupt (send the process an interrupt) and
> -// Kill (force the process to exit).
> -var (
> -       Interrupt Signal = syscall.SIGINT
> -       Kill      Signal = syscall.SIGKILL
> -)
> -
>  // Getpid returns the process id of the caller.
>  func Getpid() int { return syscall.Getpid() }
>
> Index: src/pkg/os/exec_plan9.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec_plan9.go
> +++ b/src/pkg/os/exec_plan9.go
> @@ -11,6 +11,11 @@
>        "time"
>  )
>
> +var (
> +       Interrupt = syscall.Note("interrupt")
> +       Kill      = syscall.Note("kill")
> +)
> +
>  func startProcess(name string, argv []string, attr *ProcAttr) (p
> *Process, err error) {
>        sysattr := &syscall.ProcAttr{
>                Dir: attr.Dir,
> @@ -30,35 +35,35 @@
>        return newProcess(pid, h), nil
>  }
>
> -// Plan9Note implements the Signal interface on Plan 9.
> -type Plan9Note string
> -
> -func (note Plan9Note) String() string {
> -       return string(note)
> +func (p *Process) writeProcFile(file string, data string) error {
> +       f, e := OpenFile("/proc/"+itoa(p.Pid)+**"/ctl", O_WRONLY, 0)
> +       if e != nil {
> +               return e
> +       }
> +       defer f.Close()
> +       _, e = f.Write([]byte(data))
> +       return e
>  }
>
>  func (p *Process) signal(sig Signal) error {
>        if p.done {
>                return errors.New("os: process already finished")
>        }
> -
> -       f, e := OpenFile("/proc/"+itoa(p.Pid)+**"/note", O_WRONLY, 0)
> -       if e != nil {
> +       if sig == Kill {
> +               // Special-case the kill signal since it doesn't use
> /proc/$pid/note.
> +               return p.Kill()
> +       }
> +       if e := p.writeProcFile("note", sig.String()); e != nil {
>                return NewSyscallError("signal", e)
>        }
> -       defer f.Close()
> -       _, e = f.Write([]byte(sig.String()))
> -       return e
> +       return nil
>  }
>
>  func (p *Process) kill() error {
> -       f, e := OpenFile("/proc/"+itoa(p.Pid)+**"/ctl", O_WRONLY, 0)
> -       if e != nil {
> +       if e := p.writeProcFile("ctl", "kill"); e != nil {
>                return NewSyscallError("kill", e)
>        }
> -       defer f.Close()
> -       _, e = f.Write([]byte("kill"))
> -       return e
> +       return nil
>  }
>
>  func (p *Process) wait() (ps *ProcessState, err error) {
> Index: src/pkg/os/exec_posix.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec_posix.go
> +++ b/src/pkg/os/exec_posix.go
> @@ -10,6 +10,11 @@
>        "syscall"
>  )
>
> +var (
> +       Interrupt Signal = syscall.SIGINT
> +       Kill      Signal = syscall.SIGKILL
> +)
> +
>  func startProcess(name string, argv []string, attr *ProcAttr) (p
> *Process, err error) {
>        // Double-check existence of the directory we want
>        // to chdir into.  We can make the error clearer this way.
> Index: src/pkg/syscall/syscall_plan9.**go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_**plan9.go
> +++ b/src/pkg/syscall/syscall_**plan9.go
> @@ -23,6 +23,16 @@
>  // NewError converts s to an ErrorString, which satisfies the Error
> interface.
>  func NewError(s string) error { return ErrorString(s) }
>
> +// A Note is a string describing a process signal.
> +// It implements the os.Signal interface.
> +type Note string
> +
> +func (n Note) Signal() {}
> +
> +func (n Note) String() string {
> +       return string(n)
> +}
> +
>  var (
>        Stdin  = 0
>        Stdout = 1
> @@ -335,14 +345,6 @@
>        return make([]int, 0), nil
>  }
>
> -type Signal int
> -
> -func (s Signal) Signal() {}
> -
> -func (s Signal) String() string {
> -       return ""
> -}
> -
>  //sys  Dup(oldfd int, newfd int) (fd int, err error)
>  //sys  Open(path string, mode int) (fd int, err error)
>  //sys  Create(path string, mode int, perm uint32) (fd int, err error)
> Index: src/pkg/syscall/zerrors_plan9_**386.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/zerrors_**plan9_386.go
> +++ b/src/pkg/syscall/zerrors_**plan9_386.go
> @@ -24,9 +24,6 @@
>        S_IFREG  = 0x8000
>        S_IFLNK  = 0xa000
>        S_IFSOCK = 0xc000
> -
> -       SIGINT  = Signal(0x2)
> -       SIGKILL = Signal(0x9)
>  )
>
>  // Errors
>
>
>

[ality, 2012-04-17 01:10:38]

Brad Fitzpatrick <bradfitz@golang.org> once said:
> godoc won't look good with this (the docs on the vars will be sorted away
> in a different section with the Signal interface type).  You may just have
> to duplicate the docs between exec_posix.go and exec_plan9.go.

I tried "godoc os Signal", "godoc os Interrupt", and
compared the web interface and it looks fine to me.

The variables always end up below the Signal type docs.

What am I missing?

  Anthony

[akumar, 2012-04-17 07:25:25]

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec_plan9.go
File src/pkg/os/exec_plan9.go (right):

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec_plan9.go#newc...
src/pkg/os/exec_plan9.go:38: func (p *Process) writeProcFile(file string, data
string) error {
Make this use ``file''; currently you are just writing everything to the
process' ctl file.

[rsc, 2012-04-25 02:55:52]

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec.go#newcode52
src/pkg/os/exec.go:52: //
Please move this comment back to the var blocks.
If nothing else the comment should be on the var block for when
programmers are reading the source code.

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec_plan9.go
File src/pkg/os/exec_plan9.go (right):

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec_plan9.go#newc...
src/pkg/os/exec_plan9.go:14: var (
Copy the doc comment here, please.

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec_plan9.go#newc...
src/pkg/os/exec_plan9.go:15: Interrupt = syscall.Note("interrupt")
Interrupt Signal =
Kill Signal = 

they need to have os.Signal type, not syscall.Note type.

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec_posix.go
File src/pkg/os/exec_posix.go (right):

http://codereview.appspot.com/6015046/diff/1002/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:13: var (
Copy the doc comment here too.

[ality, 2012-04-26 10:05:43]

Hello bradfitz@golang.org, seed@mail.nanosouffle.net, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[akumar, 2012-04-26 22:35:38]

https://codereview.appspot.com/6015046/diff/11001/src/pkg/os/exec_plan9.go
File src/pkg/os/exec_plan9.go (right):

https://codereview.appspot.com/6015046/diff/11001/src/pkg/os/exec_plan9.go#ne...
src/pkg/os/exec_plan9.go:42: f, e := OpenFile("/proc/"+itoa(p.Pid)+"/ctl",
O_WRONLY, 0)
f, e := OpenFile("/proc/"+itoa(p.Pid)+file, O_WRONLY, 0)
(You still haven't fixed this yet.)

[ality, 2012-04-27 05:35:28]

PTAL

https://codereview.appspot.com/6015046/diff/11001/src/pkg/os/exec_plan9.go
File src/pkg/os/exec_plan9.go (right):

https://codereview.appspot.com/6015046/diff/11001/src/pkg/os/exec_plan9.go#ne...
src/pkg/os/exec_plan9.go:42: f, e := OpenFile("/proc/"+itoa(p.Pid)+"/ctl",
O_WRONLY, 0)
On 2012/04/26 22:35:38, akumar wrote:
> f, e := OpenFile("/proc/"+itoa(p.Pid)+file, O_WRONLY, 0)
> (You still haven't fixed this yet.)

Wow. I don't know how I missed your comment.
Apologies.

Done.

[rsc, 2012-05-03 21:44:26]

LGTM

http://codereview.appspot.com/6015046/diff/20001/src/pkg/syscall/syscall_plan...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/6015046/diff/20001/src/pkg/syscall/syscall_plan...
src/pkg/syscall/syscall_plan9.go:26: // A Note is a string describing a process
signal.
s/signal/note/

[akumar, 2012-05-03 22:11:03]

LGTM.

On 2012/05/03 21:44:26, rsc wrote:
> LGTM
> 
>
http://codereview.appspot.com/6015046/diff/20001/src/pkg/syscall/syscall_plan...
> File src/pkg/syscall/syscall_plan9.go (right):
> 
>
http://codereview.appspot.com/6015046/diff/20001/src/pkg/syscall/syscall_plan...
> src/pkg/syscall/syscall_plan9.go:26: // A Note is a string describing a
process
> signal.
> s/signal/note/

[ality, 2012-05-04 10:44:51]

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

syscall: remove the Signal type on Plan 9

Instead use a new type, "Note", whose underlying
type is just a string.  This change allows us to
remove the exported os.Plan9Note type.

R=bradfitz, seed, rsc
CC=golang-dev
http://codereview.appspot.com/6015046