code review 6261053: syscall: correct Win32finddata definition

syscall: correct Win32finddata definition

Fixes issue 3685.

[brainman, 2012-05-31 01:49:23]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[brainman, 2012-05-31 01:49:44]

This breaks "go api" check, and I do not know what to do. Suggestions are
welcome.

Alex

[rsc, 2012-05-31 12:32:11]

It's annoying, but I think we need to define new types and functions.
Specifically:

1. Keep the old Win32finddata as is, but add a comment:
// Win32finddata is an incorrect struct definition, preserved for
// backwards compatibility. Use Win32finddata1 and the
// FindFirstFile1 and FindNextFile1 functions instead.
type Win32finddata struct ...

2. Add the correct definition as type Win32finddata1.

3. Keep
syscall_windows.go:132: //sys	FindFirstFile(name *uint16, data
*Win32finddata) (handle Handle, err error) [failretval==InvalidHandle]
= FindFirstFileW
syscall_windows.go:133: //sys	FindNextFile(handle Handle, data
*Win32finddata) (err error) = FindNextFileW

4. Add two more lines for FindFirstFile1 and FindNextFile1, with the
new type for data.

Russ

[brainman, 2012-06-01 05:43:36]

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

Please take another look.

[rsc, 2012-06-02 16:51:49]

LGTM

[brainman, 2012-06-03 09:27:29]

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

syscall: correct Win32finddata definition

Fixes issue 3685.

R=golang-dev, rsc
CC=golang-dev
http://codereview.appspot.com/6261053

[frichter, 2012-06-07 02:57:13]

Sorry for the 25th hour feedback.  I just noticed this CL is being considered
for Go1.0.2, and I think that the approach here was wrong. 

I believe it violates the spirit of the "go1" promise "Code that compiles in Go
1 should, with few exceptions, continue to compile and run throughout the
lifetime of that version, even as we issue updates and bug fixes ..."

Code that used to compile and run (regardless of the potential for memory
smashing) will now compile and NO LONGER run.  But the user won't know until
runtime.

Code that didn't run, still compiles and still won't work properly without a
source code change that the user has no way to know is required until he gets a
new runtime error.

Code that does not yet exist could, in theory, try to use the still existing,
but now "disabled" API (it will still show up in the docs won't it?), and then
have to be changed again when it fails at runtime.

None of these seems intuitive or programmer friendly.


Alex's original patch set 3 would appear to have been the proper choice for any
of these cases; people will just recompile existing code and it would "just
work".  Fix go1.txt to make the API checker happy with the actual Windows data
structure.

This is not an API "change", it would just mean bringing go1.txt in line with
reality for an OS specific data structure.

Or perhaps I am missing some bigger issue?

[rsc, 2012-06-07 04:01:29]

You may be right. There is also CL 6244051. I'm going to talk to
Andrew and others about it next week.

Russ

[brainman, 2012-06-07 04:17:34]

On 2012/06/07 04:01:29, rsc wrote:
> You may be right. ...

I think he is. This case is broken any way you look at it:

1) If you take my patch #3, then some old programs might not compile;

2) If you take submitted change, then these programs will compile, but will
certainly crash during execution.

So, it is a question of one out of 2 evils. I prefer 1).

Alex

[rsc, 2012-06-07 04:23:18]

On Thu, Jun 7, 2012 at 12:17 AM,  <alex.brainman@gmail.com> wrote:
> 2) If you take submitted change, then these programs will compile, but
> will certainly crash during execution.

I would have preferred your change to leave the old behavior, so that
someone who wanted to be careful could still write code that worked
with the original Go 1 structures (by interpreting the byte arrays
themselves). That might still be worth doing.

Russ

[brainman, 2012-06-07 04:32:28]

On 2012/06/07 04:23:18, rsc wrote:
> 
> ... who wanted to be careful ...

Isn't Go supposed to hold our hand here? It is very hard to think about bugs in
your program, when you know that there are memory corruption issues in it.

> ... That might still be worth doing.

Fair enough. But I still do not see how. Unless, you just want to honor "you
program will compile" promise.

Alex

[rsc, 2012-06-07 04:44:21]

You're right - the old struct is useless and would have caused crashes
(just mysterious ones). I misremembered the change. I thought it was

[N]byte
[M]byte

->

[N+1]byte
[M-1]byte

but it is ->

[N+1]byte
[M+1]byte

Russ

[brainman, 2012-06-07 04:46:42]

On 2012/06/07 04:44:21, rsc wrote:
> You're right - the old struct is useless and would have caused crashes
> (just mysterious ones). I misremembered the change. I thought it was
> 
> [N]byte
> [M]byte
> 
> ->
> 
> [N+1]byte
> [M-1]byte
> 
> but it is ->
> 
> [N+1]byte
> [M+1]byte
> 

Major cr...p, I know. I am all ashamed. :-)

Alex

[rsc, 2012-06-08 17:48:52]

On Wed, Jun 6, 2012 at 10:57 PM,  <hcwfrichter@gmail.com> wrote:
> Sorry for the 25th hour feedback.  I just noticed this CL is being
> considered for Go1.0.2, and I think that the approach here was wrong.

It's certainly true that we shouldn't do this for Go 1.0.2. Thank you
for reminding us.

I have sent out CL 6298063, which hides the new system calls and
structures and implements the old ones using them. This avoids any
visible API change and still gives access to the 8.3 name. For Go 1.1,
we might do something to avoid the overhead of the translation.

Russ