Add exported error constants for all error codes. #643
Conversation
puellanivis
left a comment
puellanivis
left a comment
There was a problem hiding this comment.
Error codes starting from 32 can also possibly be returned from the server. But we’re not supporting them because they are not yet defined.
In a similar vein, this package specifically only supports https://filezilla-project.org/specs/draft-ietf-secsh-filexfer-02.txt for which these errors are also not defined, and so no server using this package should be returning them, and since this package’s client has not negotiated an upgrade of version, a properly compliant server should also never return these codes.
If anyone does want to use these extended error codes, then I would recommend they instead switch to the dev-v2 branch, which has taken an intentional approach towards supporting these errors in its encoding/ssh/filexfer package.
request-errors.go
Outdated
| ErrSshFxNoConnection = ErrSSHFxNoConnection | ||
| ErrSshFxConnectionLost = ErrSSHFxConnectionLost | ||
| ErrSshFxOpUnsupported = ErrSSHFxOpUnsupported | ||
| ErrSshFxInvalidHandle = ErrSSHFxInvalidHandle |
There was a problem hiding this comment.
These error codes are deprecated. We should not be adding any new ones.
request-errors.go
Outdated
| case ErrSSHFxOk: | ||
| return "OK" | ||
| return "Success" | ||
| case ErrSSHFxEOF: | ||
| return "EOF" | ||
| return "An attempt to read past the end-of-file was made; or, there are no more directory entries to return." | ||
| case ErrSSHFxNoSuchFile: | ||
| return "no such file" | ||
| return "A reference was made to a file which does not exist." | ||
| case ErrSSHFxPermissionDenied: | ||
| return "permission denied" | ||
| return "The user does not have sufficient permissions to perform the operation." | ||
| case ErrSSHFxFailure: | ||
| return "An error occurred, but no specific error code exists to describe the failure." | ||
| case ErrSSHFxBadMessage: | ||
| return "bad message" | ||
| return "A badly formatted packet or other SFTP protocol incompatibility was detected." | ||
| case ErrSSHFxNoConnection: | ||
| return "no connection" | ||
| return "There is no connection to the server." | ||
| case ErrSSHFxConnectionLost: | ||
| return "connection lost" | ||
| return "The connection to the server was lost." | ||
| case ErrSSHFxOpUnsupported: | ||
| return "operation unsupported" | ||
| return "An attempted operation could not be completed by the server because the server does not support the operation." |
There was a problem hiding this comment.
Do not change the error strings returned by any previously provided errors, as this would be a breaking change of behavior.
request-errors.go
Outdated
| case ErrSSHFxInvalidHandle: | ||
| return "The handle value was invalid." | ||
| case ErrSSHFxNoSuchPath: | ||
| return "The file path does not exist or is invalid." | ||
| case ErrSSHFxFileAlreadyExists: | ||
| return "The file already exists." | ||
| case ErrSSHFxWriteProtect: | ||
| return "The file is on read-only media, or the media is write protected." | ||
| case ErrSSHFxNoMedia: | ||
| return "The requested operation cannot be completed because there is no media available in the drive." | ||
| case ErrSSHFxNoSpaceOnFilesystem: | ||
| return "The requested operation cannot be completed because there is insufficient free space on the filesystem." | ||
| case ErrSSHFxQuotaExceeded: | ||
| return "The operation cannot be completed because it would exceed the user's storage quota." | ||
| case ErrSSHFxUnknownPrincipal: | ||
| return "A principal referenced by the request (either the 'owner', 'group', or 'who' field of an ACL), was unknown." | ||
| case ErrSSHFxLockConflict: | ||
| return "The file could not be opened because it is locked by another process." | ||
| case ErrSSHFxDirNotEmpty: | ||
| return "The directory is not empty." | ||
| case ErrSSHFxNotADirectory: | ||
| return "The specified file is not a directory." | ||
| case ErrSSHFxInvalidFilename: | ||
| return "The filename is not valid." | ||
| case ErrSSHFxLinkLoop: | ||
| return "Too many symbolic links encountered or, an SSH_FXF_NOFOLLOW open encountered a symbolic link as the final component." | ||
| case ErrSSHFxCannotDelete: | ||
| return "The file cannot be deleted." | ||
| case ErrSSHFxInvalidParameter: | ||
| return "One of the parameters was out of range, or the parameters specified cannot be used together." | ||
| case ErrSSHFxFileIsADirectory: | ||
| return "The specified file was a directory in a context where a directory cannot be used." | ||
| case ErrSSHFxByteRangeLockConflict: | ||
| return "A read or write operation failed because another process's mandatory byte-range lock overlaps with the request." | ||
| case ErrSSHFxByteRangeLockRefused: | ||
| return "A request for a byte range lock was refused." | ||
| case ErrSSHFxDeletePending: | ||
| return "An operation was attempted on a file for which a delete operation is pending." | ||
| case ErrSSHFxFileCorrupt: | ||
| return "The file is corrupt; an filesystem integrity check should be run." | ||
| case ErrSSHFxOwnerInvalid: | ||
| return "The principal specified can not be assigned as an owner of a file." | ||
| case ErrSSHFxGroupInvalid: | ||
| return "The principal specified can not be assigned as the primary group of a file." | ||
| case ErrSSHFxNoMatchingByteRangeLock: | ||
| return "The requested operation could not be completed because the specified byte range lock has not been granted." |
There was a problem hiding this comment.
https://go.dev/wiki/CodeReviewComments#error-strings Do not capitalize error strings, and do not replace short simple codes with long verbose explanations of those errors.
There was a problem hiding this comment.
I have removed the capitalization and the dot at the end. As for the text, I think there's an opportunity to return a meaningful message if we do not need to care about backwards compatibility. That said, if you insist, I can put in some boring names based on the error constant.
There was a problem hiding this comment.
As for the text, I think there's an opportunity to return a meaningful message if we do not need to care about backwards compatibility.
Hyrum’s law dictates that we cannot ever know exactly who is depending on what. That’s why I’m recommending keeping the old strings. If you’ll notice, dev-v2 switches the error strings. This is the appropriate place to recommend new error strings, where the v1 compat guarantees are not applicable.
With the original strings for the original messages in place, now the overly verbose new messages do not fit in context. While I can understand the depiction of these strings as “boring”, that’s what is already here. Having such a dichotomy between the two kinds of errors is dissonant, and a violation of the principle of least surprise.
The error codes starting from 9 are defined internally and can be returned by the server, but without exported error constants it's impractical to match on and handle these errors. This change adds Err constants and text error strings for these codes.
|
I repeat that adding this feature to the With a |
|
Some of my colleagues use this package, to some extent because I used it 10 years ago, to talk to various network devices (and you know how quirky the protocol support can be for these; there should be a commit in the history with my name on it related to that). It's your decision after all, if this is not accepted I'll just update my private fork. Thanks. |
|
Yeah, I understand how simple the change is, and why it might be useful… it’s just more of a “yeah, but technically all of those are explicitly unsupported by the version we’re supporting.” They might be defined, but also… not really. 😩 It’s a rough choice especially when just about everything stopped supporting any version except the one we’re on. Like, I believe they’re trying to start up a newest version, and the plan is basically to reset back to this version, because no one was really ever supporting anything beyond this version. 🤦♀️ @drakkan What’s your perspective on this? I don’t want to be heavy handed here, especially when I’m conflicted even myself. |
2 participants
The error codes starting from 9 are defined internally and can be returned by the server, but without exported error constants it's impractical to match on and handle these errors.
This change adds Err constants and text error strings for these codes.