code review 1846044: syscall Pipe for Windows

syscall Pipe for Windows
Does not fix ForkExec

[peterGo, 2010-07-15 22:28:54]

Hello rsc, brainman (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-07-15 23:05:36]

http://codereview.appspot.com/1846044/diff/2001/3001
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1846044/diff/2001/3001#newcode248
src/pkg/syscall/syscall_windows.go:248: ft, errno := GetFileType(uint32(fd))
I'm surprised SetFilePointer doesn't return an error for pipes.
What does it do?  Just silently fail?

[brainman, 2010-07-15 23:56:26]

On 2010/07/15 22:28:54, PeterGo wrote:
> I'd like you to review this change.

Do you aware of existing CL:

http://codereview.appspot.com/1715046/show

?


Alex

[brainman, 2010-07-15 23:57:54]

> I'm surprised SetFilePointer doesn't return an error for pipes.
> What does it do?  Just silently fail?

As from http://msdn.microsoft.com/en-us/library/aa365541%28VS.85%29.aspx

>

The hFile parameter must refer to a file stored on a seeking device; for
example, a disk volume. Calling the SetFilePointer function with a handle to a
non-seeking device such as a pipe or a communications device is not supported,
even though the SetFilePointer function may not return an error. The behavior of
the SetFilePointer function in this case is undefined. 

<

[peterGo, 2010-07-15 23:58:12]

http://codereview.appspot.com/1846044/diff/2001/3001
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1846044/diff/2001/3001#newcode248
src/pkg/syscall/syscall_windows.go:248: ft, errno := GetFileType(uint32(fd))
On 2010/07/15 23:05:36, rsc1 wrote:
> I'm surprised SetFilePointer doesn't return an error for pipes.
> What does it do?  Just silently fail?

It's silently undefined!

The hFile parameter must refer to a file stored on a seeking device; for
example, a disk volume. Calling the SetFilePointer function with a handle to a
non-seeking device such as a pipe or a communications device is not supported,
even though the SetFilePointer function may not return an error. The behavior of
the SetFilePointer function in this case is undefined.

SetFilePointer Function
http://msdn.microsoft.com/en-us/library/aa365541%28VS.85%29.aspx

[peterGo, 2010-07-16 04:29:11]

On 2010/07/15 23:56:26, brainman wrote:
> On 2010/07/15 22:28:54, PeterGo wrote:
> > I'd like you to review this change.
> 
> Do you aware of existing CL:
> 
> http://codereview.appspot.com/1715046/show

No, I don't subscribe to golang-dev.

The patch failed to apply. Reading the code, I see some things that need to be
fixed.

[brainman, 2010-07-18 23:54:39]

http://codereview.appspot.com/1846044/diff/2001/3001
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1846044/diff/2001/3001#newcode398
src/pkg/syscall/syscall_windows.go:398: if errno == 0 {
Peter. Why do you need to check for errno == 0 here?

[peterGo, 2010-07-19 04:39:49]

http://codereview.appspot.com/1846044/diff/2001/3001
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1846044/diff/2001/3001#newcode398
src/pkg/syscall/syscall_windows.go:398: if errno == 0 {
On 2010/07/18 23:54:40, brainman wrote:
> Peter. Why do you need to check for errno == 0 here?

The short answer is that the syscall Pipe function returns a non-zero value of
errno, instead of a non-nil value of os.Error, to indicate an error.

  e := syscall.Pipe(p[0:])
  if e != 0 {
      syscall.ForkLock.RUnlock()
      return nil, nil, NewSyscallError("pipe", e)
  }

The CreatePipe Windows API function guarantees that: "If the function succeeds,
the return value is nonzero. If the function fails, the return value is zero."
Since ok = bool(r0 != 0), the value of ok tells us whether the CreatePipe
function call succeeded.

The GetLastError Windows API function guarantees very little. "The last-error
code is maintained on a per-thread basis. ... some functions call SetLastError
with a zero when they succeed, wiping out the error code set by the most
recently failed function. ... The error codes returned by a function are not
part of the Windows API specification and can vary by operating system or device
driver. For this reason, we cannot provide the complete list of error codes that
can be returned by each function. There are also many functions whose
documentation does not include even a partial list of error codes that can be
returned." The value of errno may be zero; we can't be certain it's not.

Since EINVAL is not zero, if ok indicates an error, errno is guaranteed to
indicate an error with a non-zero value.

  if !ok {
      if errno == 0 {
          errno = EINVAL
      }
      return
  }

[brainman, 2010-07-19 04:52:08]

On 2010/07/19 04:39:49, PeterGo wrote:

> ... The value of errno may be zero; we can't be certain it's not.
> 

As from CreatePipe doco:

>

Return Value

If the function succeeds, the return value is nonzero.

If the function fails, the return value is zero. To get extended error
information, call GetLastError.

<

I read this as a implied promise that GetLastError will return us an error
number (non-zero) in case of CreatePipe failure. Am I missing something here?


Alex

[peterGo, 2010-07-20 03:23:47]

On 2010/07/19 04:52:08, brainman wrote:
> On 2010/07/19 04:39:49, PeterGo wrote:
> 
> > ... The value of errno may be zero; we can't be certain it's not.
> > 
> 
> As from CreatePipe doco:
> 
> >
> 
> Return Value
> 
> If the function succeeds, the return value is nonzero.
> 
> If the function fails, the return value is zero. To get extended error
> information, call GetLastError.
> 
> <
> 
> I read this as a implied promise that GetLastError will return us an error
> number (non-zero) in case of CreatePipe failure. Am I missing something here?
> 
> 
> Alex

Alex

The only thing that GetLastError function guarantees is that it will return the
32-bit last-error code for the thread which was set by the most recent call to
the SetLastError or SetLastErrorEx function.

"Note that some functions call SetLastError or SetLastErrorEx with 0 when they
succeed, wiping out the error code set by the most recently failed function,
while others do not."

Therefore, there is no promise, and certainly no guarantee, that the last-error
code is non-zero.

Last-Error Code
http://msdn.microsoft.com/en-us/library/ms680347%28VS.85%29.aspx

Peter

[peterGo, 2010-07-20 03:28:28]

If the other link is corrupted, try this one.

Last-Error Code (Windows)
http://msdn.microsoft.com/en-us/library/ms680347.aspx

[brainman, 2010-07-20 03:43:26]

> The only thing that GetLastError function guarantees is that it will return
the
> 32-bit last-error code for the thread which was set by the most recent call to
> the SetLastError or SetLastErrorEx function.
> 
> "Note that some functions call SetLastError or SetLastErrorEx with 0 when they
> succeed, wiping out the error code set by the most recently failed function,
> while others do not."
> 
> Therefore, there is no promise, and certainly no guarantee, that the
last-error
> code is non-zero.
> 

So, you're suggesting that:

1) CreatePipe doesn't set "lasterror";

or

2) "Lasterror" gets changed between we call CreatePipe and GetLastError;

or

3) both 1) and 2).

Which?


Alex

[peterGo, 2010-07-21 05:26:29]

Alex,

You are missing the point entirely; by design, it doesn't matter why. Whatever
the value of errno returned by the GetLastError function, the Pipe function will
return a valid value for errno. This code is always correct. A good primer on
this topic is Systematic Programming: An Introduction by Niklaus Wirth.

What is the value of gle for this code fragment?

 	syscall.SetLastError(7)

	gle := syscall.GetLastError()

	fmt.Println(gle)


The answer is some number n, where 0 <= n < 2^32. Does that surprise you? It
shouldn't.

Peter
Peter

[brainman, 2010-07-21 06:04:01]

> You are missing the point entirely; ...

Perhaps. But I would like to see where my problem is <g>.

> ... Whatever
> the value of errno returned by the GetLastError function, the Pipe function
will
> return a valid value for errno. This code is always correct. 

True. But, I insist, it will work as well as without your check errno == 0. I am
just surprised, we need this code that does nothing!

> What is the value of gle for this code fragment?
> 
>  	syscall.SetLastError(7)
> 
> 	gle := syscall.GetLastError()
> 
> 	fmt.Println(gle)
> 
> 
> The answer is some number n, where 0 <= n < 2^32. 

True. (Assuming you're talking real go program here).

> ... Does that surprise you? 

No, it doesn't

> ... It
> shouldn't.
> 

Well, it doesn't.


Alex

[peterGo, 2010-07-22 01:00:53]

Alex,

This is a CL, which not the place to explain OS internals in detail, so I'll be
brief.

It's quite simple; there's no mechanism in place to stop the last error value
being changed to any value, including zero. You might think that, if you quickly
ask for the last error value, nobody will get a chance to change it; that's not
true; what is stopping them?

Even if the problem doesn't appear under normal circumstances, a debugger often
scrambles the state, thereby creating unreproducible, unpredictable, and
frustrating phantom errors. It also may not appear during normal testing, but
appear intermittently under heavy, fluctuating production loads. Your software
should be reliable enough to fly a passenger airplane or run a critical care
medical device i.e have zero defects.

Peter

[brainman, 2010-07-22 01:33:01]

On 2010/07/22 01:00:53, PeterGo wrote:

> It's quite simple; there's no mechanism in place to stop the last error value
> being changed to any value, including zero. You might think that, if you
quickly
> ask for the last error value, nobody will get a chance to change it; that's
not
> true; what is stopping them?
> 

If you look at the actual code that does these calls:

http://golang.org/src/pkg/runtime/windows/thread.c

void
call_syscall(void *args)
{
	StdcallParams *p = (StdcallParams*)args;
	p->r = (uintptr)stdcall_raw((void*)p->fn, p->args[0], p->args[1], p->args[2],
p->args[3], p->args[4], p->args[5], p->args[6], p->args[7], p->args[8]);
	p->err = (uintptr)stdcall_raw(GetLastError);
	return;
}

you will see, that CreatePipe call will be immediately followed by GetLastError.
There is nothing between them to change "last error" value. I say, there is no
way for "somebody" to change it. If you think otherwise, then one of us is
mistaken. If I'm wrong, then I don't understand what is happening here. That
would be a problem for me.


Alex

[Joe Poirier, 2010-07-22 03:37:00]

On 2010/07/22 01:33:01, brainman wrote:
> On 2010/07/22 01:00:53, PeterGo wrote:
> 
> > It's quite simple; there's no mechanism in place to stop the last error
value
> > being changed to any value, including zero. You might think that, if you
> quickly
> > ask for the last error value, nobody will get a chance to change it; that's
> not
> > true; what is stopping them?
> > 
> 
> If you look at the actual code that does these calls:
> 
> http://golang.org/src/pkg/runtime/windows/thread.c
> 
> void
> call_syscall(void *args)
> {
> 	StdcallParams *p = (StdcallParams*)args;
> 	p->r = (uintptr)stdcall_raw((void*)p->fn, p->args[0], p->args[1], p->args[2],
> p->args[3], p->args[4], p->args[5], p->args[6], p->args[7], p->args[8]);
> 	p->err = (uintptr)stdcall_raw(GetLastError);
> 	return;
> }
> 
> you will see, that CreatePipe call will be immediately followed by
GetLastError.
> There is nothing between them to change "last error" value. I say, there is no
> way for "somebody" to change it. If you think otherwise, then one of us is
> mistaken. If I'm wrong, then I don't understand what is happening here. That
> would be a problem for me.
> 
> 
> Alex

It is possible for the returning windows function to set the errno value to zero
for a failure then clear the error code accessed by GetLastError by calling
SetLastError(0). I've seen this happen when calling fopen on Windows 2000 and CE
both. 

Here's some community feedback on msdn (http://tinyurl.com/266n2xb) about not
relying on GetLastError: 

"Handling Shell_NotifyIcon failure: Unfortunately, you cannot rely on the error
code returned by GetLastError. When Shell_NotifyIcon returns false, some of the
common errors returned by GetLastError are:
ERROR_FILE_NOT_FOUND (2)
ERROR_TIMEOUT (1460)
ERROR_SUCCESS (0)"

IMHO, GetLastError codes should only be used for informational purposes, e.g.
with FormatMessage, when a dll function call fails.

-joe

[brainman, 2010-07-22 03:59:23]

On 2010/07/22 03:37:00, Joe Poirier wrote:

> It is possible for the returning windows function to set the errno value to
zero
> for a failure then clear the error code accessed by GetLastError by calling
> SetLastError(0). I've seen this happen when calling fopen on Windows 2000 and
CE
> both. 

Do you have any references, some factual programs I can run to test? I do have
2000.

> Here's some community feedback on msdn (http://tinyurl.com/266n2xb) about not
> relying on GetLastError: 
> 

Nowhere in Shell_NotifyIcon doco I can see a statement that error number could
be retrieved by GetLastError(). The "community" just assumed that.

> IMHO, GetLastError codes should only be used for informational purposes, e.g.
> with FormatMessage, when a dll function call fails.

No. It's not that simple. All syscall go functions return errno != 0 on failure,
we must do the same. I believe, for everything we've used so far we could use
errno = GetLastError() on falure. If it's not the case, then we have to do what
Peter did, but not for one function, but all of them. I would like to see some
concrete evidence that our current assumption is false.


Alex

[Joe Poirier, 2010-07-22 04:39:05]

On Wed, Jul 21, 2010 at 10:59 PM,  <alex.brainman@gmail.com> wrote:
> On 2010/07/22 03:37:00, Joe Poirier wrote:
>
>> It is possible for the returning windows function to set the errno
>
> value to zero
>>
>> for a failure then clear the error code accessed by GetLastError by
>
> calling
>>
>> SetLastError(0). I've seen this happen when calling fopen on Windows
>
> 2000 and CE
>>
>> both.
>
> Do you have any references, some factual programs I can run to test? I
> do have 2000.
>
>> Here's some community feedback on msdn (http://tinyurl.com/266n2xb)
>
> about not
>>
>> relying on GetLastError:
>
>
> Nowhere in Shell_NotifyIcon doco I can see a statement that error number
> could be retrieved by GetLastError(). The "community" just assumed that.
>
>> IMHO, GetLastError codes should only be used for informational
>
> purposes, e.g.
>>
>> with FormatMessage, when a dll function call fails.
>
> No. It's not that simple. All syscall go functions return errno != 0 on
> failure, we must do the same. I believe, for everything we've used so
> far we could use errno = GetLastError() on falure. If it's not the case,
> then we have to do what Peter did, but not for one function, but all of
> them. I would like to see some concrete evidence that our current
> assumption is false.
>
>
> Alex
>
> http://codereview.appspot.com/1846044/show
>

Okay, here's another example using FindWindowEx on Windows 7 where
it does talk about using GetLastError: http://tinyurl.com/2a6lqyg. I've found
other examples of this behavior listed on Google as well.

I haven't seen a msdn windows example that uses the GetLastError code in
anything other than displaying a message, using FormatMessage, after
a function fails, ie returns a zero value.

-joe

[brainman, 2010-07-22 04:55:13]

On 2010/07/22 04:39:05, Joe Poirier wrote:

> Okay, here's another example using FindWindowEx on Windows 7 where
> it does talk about using GetLastError: http://tinyurl.com/2a6lqyg. I've found
> other examples of this behavior listed on Google as well.

I can't argue with you people <g>. I'll just change our logic to check for all
functions in zsyscall_windows_386.go. CL coming.


Alex

[Joe Poirier, 2010-07-22 04:59:39]

On Wed, Jul 21, 2010 at 11:55 PM,  <alex.brainman@gmail.com> wrote:
> On 2010/07/22 04:39:05, Joe Poirier wrote:
>
>> Okay, here's another example using FindWindowEx on Windows 7 where
>> it does talk about using GetLastError: http://tinyurl.com/2a6lqyg.
>
> I've found
>>
>> other examples of this behavior listed on Google as well.
>
> I can't argue with you people <g>. I'll just change our logic to check
> for all functions in zsyscall_windows_386.go. CL coming.
>

An added twist, there are instances where a function succeeds but
the GetLastError value is set to non-zero, eg WriteFileEx and WriteFile.

-joe

[brainman, 2010-07-22 05:01:14]

> An added twist, there are instances where a function succeeds but
> the GetLastError value is set to non-zero, eg WriteFileEx and WriteFile.

Don't care about that.


Alex

[peterGo, 2010-07-22 10:53:39]

Joe,

On Jul 22, Joseph Poirier <jdpoir...@gmail.com> wrote:

> IMHO, GetLastError codes should only be used for informational purposes,
> e.g. with FormatMessage, when a dll function call fails.

> I haven't seen a msdn windows example that uses the GetLastError code in
> anything other than displaying a message, using FormatMessage, after
> a function fails, ie returns a zero value.

That's right.  The return value, not the last error code returned by
GetLastError, is the sole means of determining success or failure for
CreatePipe and most other functions: "If the function succeeds, the
return value is nonzero. If the function fails, the return value is
zero."  "[The last] error code may tell more about what actually
occurred to make the function fail."  Or, it may not.  It's "an
additional error code."  "Any function can call the SetLastError or
SetLastErrorEx function to set the last-error code for the current
thread."  "Note that some functions call SetLastError or
SetLastErrorEx with 0 [zero] when they succeed, wiping out the error
code set by the most recently failed function, while others do not."
"The last-error code is maintained on a per-thread basis."

Peter

[peterGo, 2010-07-22 14:50:55]

Joe,

On 2010/07/22 04:59:39, Joe Poirier wrote:

> An added twist, there are instances where a function succeeds but
> the GetLastError value is set to non-zero, eg WriteFileEx and WriteFile.

That's on my list of things to fix too.

Peter

[peterGo, 2010-07-22 20:46:12]

Alex,

On 2010/07/22 01:33:01, brainman wrote:

> If you look at the actual code that does these calls:

It's good practice to minimize testing and avoid bugs by carefully reviewing all
relevant code, documentation, books, and literature before starting to design
and code a solution. Therefore, I read all the relevant code before I submitted
the CL.

> CreatePipe call will be immediately followed by GetLastError.
> There is nothing between them to change "last error" value. I say, there is no
> way for "somebody" to change it. If you think otherwise, then one of us is
> mistaken. If I'm wrong, then I don't understand what is happening here. That
> would be a problem for me.

You're mistaken.

Peter

[peterGo, 2010-07-22 20:48:06]

Alex,

On 2010/07/22 05:01:14, brainman wrote:

> > An added twist, there are instances where a function succeeds but
> > the GetLastError value is set to non-zero, eg WriteFileEx and WriteFile.

> Don't care about that.

You should care.

Peter

[rsc, 2010-07-22 21:16:49]

Please, enough of the sniping back and forth.
We're all working toward the same goal.

The code in question says

	ok, errno := CreatePipe()
	if !ok {
		if errno == 0 {
			errno = EINVAL
		}
		return
	}

The if errno == 0 does not belong here.  If CreatePipe
returns !ok, then you should assume errno != 0.
To do otherwise is a useless API.

My reading of the Windows docs is that in this case it
can't happen that CreatePipe returns false without setting
the last error, but the docs are far from clear and who
knows what the implementations do.  This kind of thing,
because the situation is murky, might be worth defending
against, but only in the general case, not at one call site
of one wrapper.  Let's fix the wrapper generator so that
when it is setting e1 it makes it EINVAL if e1 == 0.
This can happen in a separate CL; this CL should drop the check.

Thanks.
Russ

[brainman, 2010-07-22 23:31:12]

On 2010/07/22 21:16:49, rsc wrote:

> ...  This kind of thing,
> because the situation is murky, might be worth defending
> against, but only in the general case, not at one call site
> of one wrapper.  Let's fix the wrapper generator so that
> when it is setting e1 it makes it EINVAL if e1 == 0.
> This can happen in a separate CL; ...

As I said earlier, CL is on the way. Sorry for the noise.


Alex

[peterGo, 2010-07-26 02:56:19]

On 2010/07/22 21:16:49, rsc wrote:

> The code in question says
> 
> 	ok, errno := CreatePipe()
> 	if !ok {
> 		if errno == 0 {
> 			errno = EINVAL
> 		}
> 		return
> 	}
> 
> The if errno == 0 does not belong here.  If CreatePipe
> returns !ok, then you should assume errno != 0.

Done

> Let's fix the wrapper generator so that
> when it is setting e1 it makes it EINVAL if e1 == 0.
> This can happen in a separate CL; this CL should drop the check.

Done and checked in: http://codereview.appspot.com/1872045

[peterGo, 2010-07-26 02:56:54]

Hello rsc, brainman, Joe Poirier (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-07-26 03:35:56]

Peter,

Unfortunately, I won't be able to accept your CL, because I will accept
http://codereview.appspot.com/1666045/show instead. CL166045 was created first,
and it is only fair to the author to accept his.

I'm sorry this happened. Perhaps in the future we should talk to each other
more, so we all know what is going on.

Thank you.


Alex

[rsc1, 2010-08-03 20:02:46]

removing reviewers