code review 5673077: os: replace non-portable Waitmsg with portable ProcessState

os: replace non-portable Waitmsg with portable ProcessState
Use methods for key questions.
Provide access to non-portable pieces through portable methods.
Windows and Plan 9 updated.

[r, 2012-02-17 03:36:24]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-02-17 03:42:14]

This is half the battle.

The other half is http://code.google.com/p/go/issues/detail?id=2823 ...
that the "options int" argument to Wait is a mix of random portable &
non-portable bits.


On Fri, Feb 17, 2012 at 2:36 PM, <r@golang.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:
> os: proposal for Waitmsg replacement
> not ready for checkin; no downstream work is done
> (although go test os passes on darwin)
> - call it ProcessState
> - use methods for key questions
> - provide access to non-portable pieces through portable methods
>
> Please review this at
http://codereview.appspot.com/**5673077/<http://codereview.appspot.com/5673077/>
>
> Affected files:
>  M src/pkg/os/exec_posix.go
>  M src/pkg/os/exec_unix.go
>  M src/pkg/os/os_test.go
>
>
>

[bradfitzgoog, 2012-02-17 03:45:19]

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

http://codereview.appspot.com/5673077/diff/1/src/pkg/os/exec_posix.go#newcode89
src/pkg/os/exec_posix.go:89: func (p *ProcessState) SysTime() time.Duration {
SystemTime might be better name? Giving it the "Sys" prefix makes it look
system-dependent, like SysStatus and SysTime and FileInfo.Sys.

[r2, 2012-02-17 03:47:02]

On Feb 17, 2012, at 2:45 PM, bradfitz@google.com wrote:

> 
> http://codereview.appspot.com/5673077/diff/1/src/pkg/os/exec_posix.go
> File src/pkg/os/exec_posix.go (right):
> 
>
http://codereview.appspot.com/5673077/diff/1/src/pkg/os/exec_posix.go#newcode89
> src/pkg/os/exec_posix.go:89: func (p *ProcessState) SysTime()
> time.Duration {
> SystemTime might be better name? Giving it the "Sys" prefix makes it
> look system-dependent, like SysStatus and SysTime and FileInfo.Sys.

good point. i'll change it.

regarding Wait: different topic i'll address separately

[r, 2012-02-17 03:47:43]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, bradfitz@google.com,
r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2012-02-17 03:50:57]

Looks nice to me.

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

http://codereview.appspot.com/5673077/diff/1/src/pkg/os/exec_posix.go#newcode53
src/pkg/os/exec_posix.go:53: // ProcessState stores information about process as
reported by Wait.
s/process as reported by Wait/a process/?

http://codereview.appspot.com/5673077/diff/1/src/pkg/os/exec_posix.go#newcode61
src/pkg/os/exec_posix.go:61: func (p *ProcessState) Pid() int {
PID() ?

http://codereview.appspot.com/5673077/diff/1/src/pkg/os/exec_posix.go#newcode71
src/pkg/os/exec_posix.go:71: // such as with exit status 0 on Unix.
Should this explicitly define semantics if Exited() is returning false?

[rsc1, 2012-02-17 03:59:32]

Nice.

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

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:65: // Exited returns whether the program has exited.
Long ago I think we agreed on "reports whether" but I never bothered
to convert the tree.

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:76: // SysStatus returns system-dependent exit
information about
Maybe just Sys; we've used that in a few other places.

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:78: // type, such as syscall.WaitStaus on Unix, to
access its contents.
WaitStatus

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:83: // UserTime returns the user CPU time of the exited
process.
and its children, presumably.

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:88: // SystemTime returns the system CPU time of the
exited process.
Same.

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:100: // Wait waits for process pid to exit or stop, and
then returns the
It's possible we should retire this entirely and just tell people to
use FindProcess in the rare case they are holding just a pid.
Wait only works for child processes, and those are now created
(always) via StartProcess, so this should be very rare.

Maybe if a C program forked a child process and then exec'ed
a Go program this would be necessary.

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:30: func (p *Process) Wait(options int) (ps
*ProcessState, err error) {
I think maybe we can just drop the options.
People who care about non-blocking or waiting
for a child to get SIGTTOU can use package syscall.

[iant, 2012-02-17 04:17:32]

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

http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:65: // Exited returns whether the program has exited.
This comment makes it seems that this function can be called while the process
is still running, to see if the process has stopped.

[r2, 2012-02-17 04:34:23]

On Feb 17, 2012, at 3:17 PM, iant@golang.org wrote:

> 
> http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go
> File src/pkg/os/exec_posix.go (right):
> 
>
http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
> src/pkg/os/exec_posix.go:65: // Exited returns whether the program has
> exited.
> This comment makes it seems that this function can be called while the
> process is still running, to see if the process has stopped.

Can't it? If your process is suspended but not exited, doesn't wait give you one
of these?

-rob

[rsc1, 2012-02-17 04:40:23]

On Thu, Feb 16, 2012 at 23:34, Rob 'Commander' Pike <r@google.com> wrote:
> Can't it? If your process is suspended but not exited, doesn't wait give you
one of these?

Not if we get rid of options.

Typically Exited distinguishes between the process
calling exit and the process being killed by a signal.

Russ

[iant2, 2012-02-17 04:49:10]

FYI

Rob 'Commander' Pike <r@google.com> writes:

> On Feb 17, 2012, at 3:17 PM, iant@golang.org wrote:
>
>> 
>> http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go
>> File src/pkg/os/exec_posix.go (right):
>> 
>>
http://codereview.appspot.com/5673077/diff/5001/src/pkg/os/exec_posix.go#newc...
>> src/pkg/os/exec_posix.go:65: // Exited returns whether the program has
>> exited.
>> This comment makes it seems that this function can be called while the
>> process is still running, to see if the process has stopped.
>
> Can't it? If your process is suspended but not exited, doesn't wait give you
one of these?

You're right, don't mind me.

Ian

[r, 2012-02-21 02:13:13]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, bradfitz@google.com,
r@google.com, dsymonds@golang.org, rsc@google.com, iant@golang.org,
iant@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-02-21 02:20:24]

http://codereview.appspot.com/5673077/diff/10009/src/cmd/godoc/main.go
File src/cmd/godoc/main.go (right):

http://codereview.appspot.com/5673077/diff/10009/src/cmd/godoc/main.go#newcod...
src/cmd/godoc/main.go:114: log.Printf("executing %v failed (exit status = %d)",
args, status)
status is not set here.  it compiles, but it's always just zero (the result
parameter).

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

http://codereview.appspot.com/5673077/diff/10009/src/pkg/os/exec_posix.go#new...
src/pkg/os/exec_posix.go:70: // type, such as syscall.WaitStatus on Unix, to
access its contents.
*syscall.WaitStatus

http://codereview.appspot.com/5673077/diff/10009/src/pkg/os/exec_windows.go
File src/pkg/os/exec_windows.go (right):

http://codereview.appspot.com/5673077/diff/10009/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:89: // It is always reported as 0 on Windows.
s/always/currently/ ?

I wouldn't promise it's always zero, if we plan on implementing it in the
future.

http://codereview.appspot.com/5673077/diff/10009/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:95: // It is always reported as 0 on Windows.
likewise

[r, 2012-02-21 02:50:05]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, bradfitz@google.com,
r@google.com, dsymonds@golang.org, rsc@google.com, iant@golang.org,
iant@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-02-21 02:54:22]

http://codereview.appspot.com/5673077/diff/10010/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

http://codereview.appspot.com/5673077/diff/10010/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:23: pid1, e := syscall.Wait4(p.Pid, &status, 0, nil)
I thought we were always going to request resource usage now... s/nil/&usage/

[r, 2012-02-21 03:01:50]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, bradfitz@google.com,
r@google.com, dsymonds@golang.org, rsc@google.com, iant@golang.org,
iant@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-02-21 03:07:51]

LGTM

[bradfitz, 2012-02-21 03:08:10]

I assume you'll be updating go1.tmpl at some point.

[r2, 2012-02-21 03:10:05]

On Feb 21, 2012, at 2:08 PM, bradfitz@golang.org wrote:

> I assume you'll be updating go1.tmpl at some point.

i filed an issue about that earlier today. there's a potential complexity, but
it'll get done.

-rob

[r, 2012-02-21 03:10:38]

*** Submitted as cc7630ccd586 ***

os: replace non-portable Waitmsg with portable ProcessState
Use methods for key questions.
Provide access to non-portable pieces through portable methods.
Windows and Plan 9 updated.

R=golang-dev, bradfitz, bradfitz, r, dsymonds, rsc, iant, iant
CC=golang-dev
http://codereview.appspot.com/5673077

[rsc, 2012-02-22 17:42:18]

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

http://codereview.appspot.com/5673077/diff/14004/src/pkg/os/exec_posix.go#new...
src/pkg/os/exec_posix.go:48: status *syscall.WaitStatus // System-dependent
status info.
I know I am late to this but I suspect that this should not be a pointer.  The
status is a single word on Unix and the methods are on the value.  There hasn't
been a weekly yet so we can still fix this.

[r2, 2012-02-22 20:37:51]

On 23/02/2012, at 4:42 AM, rsc@golang.org wrote:

> 
> http://codereview.appspot.com/5673077/diff/14004/src/pkg/os/exec_posix.go
> File src/pkg/os/exec_posix.go (right):
> 
>
http://codereview.appspot.com/5673077/diff/14004/src/pkg/os/exec_posix.go#new...
> src/pkg/os/exec_posix.go:48: status *syscall.WaitStatus //
> System-dependent status info.
> I know I am late to this but I suspect that this should not be a
> pointer.  The status is a single word on Unix and the methods are on the
> value.  There hasn't been a weekly yet so we can still fix this.
> 
> http://coderevi

roger wilco