lock: wrap the returned error #54
Conversation
lock/lock_windows.go
Outdated
| @@ -54,12 +54,12 @@ func (u *winUnlocker) close() { | |||
| func lockWindows(name string) (io.Closer, error) { | |||
| fi, err := os.Stat(name) | |||
| if err == nil && fi.Size() > 0 { | |||
| return nil, fmt.Errorf("can't lock file %q: %s", name, "has non-zero size") | |||
| return nil, fmt.Errorf("can't lock file %s: has non-zero size", name) | |||
There was a problem hiding this comment.
The use of %q here was likely intentional.
Using a simple %s for filenames can often lead to confusing error output: (https://play.golang.org/p/fiuJEvghF7y)
open : no such file or directory
While we can clearly see exactly why the output here is what it is in this simple case, in the real world, it can often be much more difficult to realize that there is in fact a filename in that error message.
There was a problem hiding this comment.
It's true! I put that %q there a while ago for that reason. However, when looking at it today I noticed the Unix files use %s.
The other issue though, is that while %q avoids the empty string issue, it formats to "string safely escaped with Go syntax".
In this case, it causes "fencing"(or whatever you'd call this escaping) in Windows paths. ("C:\tmp\lock" becomes "C:\\tmp\\lock")
We can avoid both though by manually providing quotations here:
https://play.golang.org/p/RO0HBOIrdz7
| return nil, fmt.Errorf("can't lock file %s: has non-zero size", name) | |
| return nil, fmt.Errorf(`can't lock file "%s": has non-zero size`, name) |
The same could be added in the lock_unix file, or their %s changed to a %q.
Which do you think would make more sense to use in the _unix file? I'm thinking manual quotes just for consistency.
There was a problem hiding this comment.
Ah yeah. Hm. An interesting dilemma there. I‘ve simply accepted that backslashes get doubled in the Windows code. I suppose I was already deadened to it after years of dealing with backslash doubling, and quadrupling, and worse while at Microsoft. 😆
I’m not sure we want to use manual quotes either though, because then it doesn’t escape quotes in the string as well. Which, sure they cannot appear in Windows filenames, but this is an error message about “cannot make such a file”, so it could still happen.
I mean, also https://play.golang.org/p/pZmjyiqj5M3 this wonderful piece of bad, but I think that one is playing way way more into the weeds of what to protect against. 🤔 Maybe we could just if name == "" { name = "" } 😆
Not sure, I’m down with going to %s, the rest of the errors do this, and so do the internal Go libraries. At least we won’t end up doing anything worse than them.
| return 0, err | ||
| } | ||
| return handle, nil | ||
| return windows.CreateFile(windows.StringToUTF16Ptr(name), 0, 0, nil, windows.OPEN_ALWAYS, FILE_ATTRIBUTE_TEMPORARY|FILE_FLAG_DELETE_ON_CLOSE, 0) |
There was a problem hiding this comment.
On error, CreateFileW (the underlying call here) returns INVALID_HANDLE_VALUE (windows.InvalidHandle) (https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew) which is defined to be -1 not 0: (https://referencesource.microsoft.com/#System.Runtime.Remoting/channels/ipc/win32namedpipes.cs,89)
I imagine this is why the if err != nil { return 0, err } is there, i.e. to normalize the return value to the zero value in the case of an error.
Though, this might not be a big deal, as the handle can be considered an undefined value in the event err != nil and so, should actually be rarely/never used. But this is a significant change of behavior that needs to be checked, and double-checked.
There was a problem hiding this comment.
i.e. to normalize the return value to the zero value in the case of an error.
I wrote this originally and think that was the rationale.
Currently, the only caller lockWindows, immediately discards the handle if err != nil.
But I figure, if we're hoisting the error values, we may want to do the same with the system error.
And let the caller decide if any special action should be taken, or just bail out if they encounter any error.
A nice thing, is that this value is explicitly defined by the WINAPI to be an invalid handle, so even if it mistakenly propagates, passing that handle to windows/WINAPI methods should detect this, and return an error themselves saying the handle passed in was invalid.
*I think this is mostly true ^
That said, I don't think CreateFile's r1 will ever return a value besides -1, and we're more interested in r2 the GetLastError value anyhow.
So it seems we can pass back both to the caller, or normalize r1.
I'm fine with either since r2 seems most important here.
There was a problem hiding this comment.
👍 sounds good. It’s also an unexported function, so we can actually fairly definitively make sure this won’t break any code.
|
I think this is is order now. Thanks for the help with the details :^) Names are getting escaped and quoted by the |
|
Indeed, I also don’t see any concerns. |
- use fmt verb %w to wrap system native error values - use fmt verb %q to quote and escape file names in error strings
|
Squashed the commits and added a description. |
|
I unfortunately also do not have any auth to make the merge. |
Context: ipfs/go-fs-lock#19
One of the libraries using the lock package is currently doing string compares on the error values to distinguish the cause of failure.
It seems like it would be better to expose the system specific errors to the caller, so that they may be used in conjuncture with
IsandAs.Thoughts on this?