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

xrdposix: interpret kXR_Unsupported as ENOTSUP instead of ENOSYS #516

Closed
wants to merge 2 commits into from

Conversation

fizmat
Copy link
Contributor

@fizmat fizmat commented May 15, 2017

Motivation
To quote errno.h comment on ENOSYS /*Invalid system call number"*/

This error code is special: arch syscall entry code will return
-ENOSYS if users try to call a syscall that doesn't exist. To keep
failures of syscalls that really do exist distinguishable from
failures due to attempts to use a nonexistent syscall, syscall
implementations should refrain from returning -ENOSYS.

There are many reasons for the server to respond kXR_Unsupported, most of them not reasonably comparable to ENOSYS.
Some systems, like FUSE, treat ENOSYS very differently from other errors.
For example, when OPEN returns ENOSYS, it's considered a success (probably intended for cases where a filesystem does not need to implement an open syscall to work).
When xrootdfs OPEN fails because the server returned kXR_Unsupported because it does not support a specific set of open flags,
it is an error and should be treated as such. The file is not ready to be used after this open.
When FUSE requests an extended attribute and the server responds kXR_Unsupported, meaning that a particular xrootd query is not supported,
FUSE interprets it as a missing getxattr syscall and never tries to call getxattr again, even for the supported attributes.
In conclusion, FUSE shows that ENOSYS has a specific meaning and can't work as a mapping of kXR_Unsupported.
Why not use ENOTSUP instead?

Change: map kXR_Unsupported to ENOTSUP when interpreting protocol responses into error codes.

Result:
Will make dealing with kXR_Usupported errors easier and more predictable.
Should not break anything in xrootd code. The following was checked:
Errno is only compared to ENOSYS is oss, not on the network side where it can expect kXR_Unsupported to be ENOSYS.
Errno is never compared to EOPNOTSUPP.
Errno is only compared to ENOTSUP in:

  • XrdFrmAdmin::Chksum, comparison for logging only
  • XrdFrmAdmin::ChksumList, interrupt printing checksums if error is not ESTALE or ENOTSUP. The change makes sense in this context and should not break this.
  • XrdFrmAdmin::ChksumPrint, logging
  • XrdFrmAdmin::ConvTest, logging
  • XrdFrmConfig::Configure
    • looks like filesystem-side, not protocol-side?
    • if it is network-side, makes sense to interpret kXR_Unsupported as ENOTSUP in this context
  • XrdOfsFile::open
    // Create the file. If ENOTSUP is returned, promote the creation to
    // the subsequent open. This is to accomodate proxy support.
    if it is networks-side, makes sense to interpret kXR_Unsupported as ENOTSUP
  • XrdSysXAttr::Copy
    • called from XrdOssCopy::Copy
    • if it is network-side, makes sense to interpret kXR_Unsupported as ENOTSUP

Motivation
To quote errno.h comment on ENOSYS /*Invalid system call number"*/
> This error code is special: arch syscall entry code will return
> -ENOSYS if users try to call a syscall that doesn't exist.  To keep
> failures of syscalls that really do exist distinguishable from
> failures due to attempts to use a nonexistent syscall, syscall
> implementations should refrain from returning -ENOSYS.

There are many reasons for the server to respond kXR_Unsupported, most of them not reasonably comparable to ENOSYS.
Some systems, like FUSE, treat ENOSYS very differently from other errors.
For example, when OPEN returns ENOSYS, it's considered a success (probably intended for cases where a filesystem does not need to implement an open syscall to work).
When xrootdfs OPEN fails because the server returned kXR_Unsupported because it does not support a specific set of open flags,
it is an error and should be treated as such. The file is not ready to be used after this open.
When FUSE requests an extended attribute and the server responds kXR_Unsupported, meaning that a particular xrootd query is not supported,
FUSE interprets it as a missing getxattr syscall and never tries to call getxattr again, even for the supported attributes.
In conclusion, FUSE shows that ENOSYS has a specific meaning and can't work as a mapping of kXR_Unsupported.
Why not use ENOTSUP instead?

Change: map kXR_Unsupported to ENOTSUP when interpreting protocol responses into error codes.

Result:
Will make dealing with kXR_Usupported errors easier and more predictable.
Should not break anything in xrootd code. The following was checked:
Errno is only compared to ENOSYS is oss, not on the network side where it can expect kXR_Unsupported to be ENOSYS.
Errno is never compared to EOPNOTSUPP.
Errno is only compared to ENOTSUP in:
 * XrdFrmAdmin::Chksum, comparison for logging only
 * XrdFrmAdmin::ChksumList, interrupt printing checksums if error is not ESTALE or ENOTSUP. The change makes sense in this context and should not break this.
 * XrdFrmAdmin::ChksumPrint, logging
 * XrdFrmAdmin::ConvTest, logging
 * XrdFrmConfig::Configure
   * looks like filesystem-side, not protocol-side?
   * if it *is* network-side, makes sense to interpret kXR_Unsupported as ENOTSUP in this context
 * XrdOfsFile::open
   // Create the file. If ENOTSUP is returned, promote the creation to
   // the subsequent open. This is to accomodate proxy support.
   if it *is* networks-side, makes sense to interpret kXR_Unsupported as ENOTSUP
 * XrdSysXAttr::Copy
   * called from XrdOssCopy::Copy
   * if it *is* network-side, makes sense to interpret kXR_Unsupported as ENOTSUP
@abh3
Copy link
Member

abh3 commented May 16, 2017

Thank you for the detailed review and recommendation. I agree that we are misusing ENOSYS and should revert to using ENOTSUP. However, the pull request only changes the error code mapping. A more complete solution would be to change all active returns of ENOSYS to ENOTSUP. This would provide for better consistency and avoid making the same mistake in the future. Would you agree?

@fizmat
Copy link
Contributor Author

fizmat commented May 16, 2017

Yes, there may be more places where ENOSYS is used but should not be. I will check and add another commit to this pull request.

@bbockelm
Copy link
Contributor

@fizmat - I noticed this one has been around for awhile. Any activity to report?

@fizmat
Copy link
Contributor Author

fizmat commented Dec 26, 2017

Sorry, I completely forgot this task was not finished, thanks for the reminder.
I will have to get back into context, but I should have things to report soon.

@abh3
Copy link
Member

abh3 commented Apr 11, 2020

This has been carried forward in it's minimal form (i.e. only addressing ENOSYS) into R5. Thank you for the pull request; is helped. However, we needed to reduce its scope.

@abh3 abh3 closed this Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants