Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: more descriptive error for File.ReadAt with negative offset #19031

Closed
ggirtsou opened this issue Feb 10, 2017 · 7 comments
Closed

os: more descriptive error for File.ReadAt with negative offset #19031

ggirtsou opened this issue Feb 10, 2017 · 7 comments
Milestone

Comments

@ggirtsou
Copy link
Contributor

@ggirtsou ggirtsou commented Feb 10, 2017

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/X6dRGzbvkW

I 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.Stat returning 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.

@bradfitz bradfitz changed the title Need a more descriptive error when too small offset is passed in os.ReadAt os: more descriptive error for File.ReadAt when too small offset is passed in? Feb 10, 2017
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 11, 2017

It's obviously OK to pass os.(*File).ReadAt an offset that is smaller than the file size. If I understand you correctly, the error was that the offset was negative, which of course is not OK. Is that correct?

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Feb 11, 2017

@ggirtsou

This comment has been minimized.

Copy link
Contributor Author

@ggirtsou ggirtsou commented Feb 11, 2017

@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 bradfitz changed the title os: more descriptive error for File.ReadAt when too small offset is passed in? os: more descriptive error for File.ReadAt with negative offset Feb 11, 2017
@bradfitz bradfitz added this to the Go1.9Maybe milestone Feb 11, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 11, 2017

Adding a check for negative seems fine.

@ggirtsou

This comment has been minimized.

Copy link
Contributor Author

@ggirtsou ggirtsou commented Feb 11, 2017

@bradfitz cool! I can start working on it and follow the process to submit my contribution.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 31, 2017

CL https://golang.org/cl/39136 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 9, 2017

CL https://golang.org/cl/40093 mentions this issue.

@gopherbot gopherbot closed this in a5999b7 Apr 10, 2017
lparth added a commit to lparth/go that referenced this issue Apr 13, 2017
…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>
@golang golang locked and limited conversation to collaborators Apr 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.