os: more descriptive error for File.ReadAt with negative offset #19031
Comments
bradfitz
changed the title
|
It's obviously OK to pass |
|
There are a lot of ways an argument to a syscall could go wrong, should we
diagnose all of the causes?
Please take a look at the man page for related syscalls and see how many
cases it could return a given error (like EINVAL in this case).
|
|
@ianlancetaylor Yes, you're right. @minux I agree there's a fine line between what the language should check for, and what the application should check. I understand there are way too many cases to account for. I think this is a case that can be improved, that's why I raised the issue. Feel free to close the issue if you think it's not worth doing it in go level. If we check this in application level, it would look like this making sure a non negative offset is passed. |
bradfitz
changed the title
|
Adding a check for negative seems fine. |
|
@bradfitz cool! I can start working on it and follow the process to submit my contribution. |
|
CL https://golang.org/cl/39136 mentions this issue. |
|
CL https://golang.org/cl/40093 mentions this issue. |
…tive offset. The existing implementation does not provide a useful error message if a negative offset is passed in File.ReadAt or File.WriteAt. This change is to return descriptive errors. An error of type *PathError is returned to keep it consistent with rest of the code. There is no need to add an exported error variable since it's used only in one file. Fixes golang#19031 Change-Id: Ib94cab0afae8c5fe4dd97ed2887018a09b9f4538 Reviewed-on: https://go-review.googlesource.com/39136 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
What version of Go are you using (
go version)?1.7.1
What operating system and processor architecture are you using (
go env)?MacOS (darwin), amd64
What did you do?
I was experimenting trying to make a go version of
tail, and made this program: https://play.golang.org/p/X6dRGzbvkWI stumbled upon a mysterious error,
read <file> invalid argument(which is different on windows as it comes from OS:read <file>: The parameter is incorrect.)What did you expect to see?
A more descriptive error, saying that provided offset is smaller than filesize.
What did you see instead?
An error that made me wonder what I'm doing wrong. Read this and thought I was shadowing a variable, but that wasn't the case.
My suggestion is to add a sanity check here: https://github.com/golang/go/blob/master/src/os/file.go#L120 if offset is smaller than file size using
os.Statreturning a more descriptive error would solve the frustration.I discussed this in gophers slack chat as well, and they thought its worth bringing this issue here. If you guys agree, I can submit a PR to improve it.
The text was updated successfully, but these errors were encountered: