-
Notifications
You must be signed in to change notification settings - Fork 380
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
Bug: wrong error message returned by Client.Remove #468
Comments
Hm… yeah, that does seem like a confusing error to be returning. Looking into the Go source code for 🤔 Hm… curious. Not sure if your proposed change would actually fix things for all cases, or just for this one case. I’ll be on a plane flying back to Germany in 24-hours, so I’ll ponder on things and try and get something going when I get back. Alternatively, I’m supposed to be working on reimplementing #432 (there were so many merge conflicts rebasing the PR, I’ve moved on to reimplementing) and this seems like something that would be good to address in that larger refactor, rather than giving too much priority into a confusing error message. 🤔 P.S.: Do you happen to know which server implementation is on the other end when you’re getting this error message? Ideally, we could address this a little better, if we knew what was generating the error. |
Hi, |
I forgot to mention that the "Not a directory" error messages (that I experienced) is always of SSH_FX_FAILURE kind. |
This is ProFTPD |
Let's say some SFTP user can properly read a file, but he doesn't have any writing permission. In my opinion, it is more than just a wrong message, because the error is claiming an incorrect reason about the removal failure. It seems to me that my proposed fix acts just like |
The problem here is that the The change you have proposed would return “is a directory” for a non-empty directory, with an error message that is now just as confusing as returning |
So, I’m starting to think that matching the implementation used by The windows implementation does a We could potentially do something similar, but this would be another roundtrip request. Although, this would save us from having to parse out the error string to figure out which ones we shouldn’t return. (i.e. something like returning the We should be able to test for the possibility of a I’ll work this into my client updates, when I get into it again. |
This seems a good and easy solution! |
Hi,
Great job! I really appreciate your effort for maintaining this library!
I mainly use your library inside a proprietary Big Data project, that is running in production since 2 years ago, using Linux VMs in the cloud, and moves many TBs of data per day.
func (c *Client) Remove(path string) error
can return an error for many reasons, of course.In most cases, the library just works fine.
If
Remove
is called for removing just a file (and not a directory), in some cases I experienced thatRemove
can return an error containing this message: "Not a directory".In my opinion, it is a bug.
Looking at the
Remove
implementation, I see that, in caseerr.Code
is equal tosshFxFailure
or if there is a permission error, the returned error is the one got fromRemoveDirectory
.I think if
Remove
is called for removing a file, it should never return an error with a message like "Not a directory".I would suggest this fixed implementation:
Thanks for your attention.
The text was updated successfully, but these errors were encountered: