Missing a few LibC Error numbers

aravindavk commented 4 years ago

Below code will not compile (Mac/Darwin returns ENOATTR and Linux returns ENODATA if xattr is not set. ENOATTR and many others are missing in the Crystal)

$ touch sample.txt
$ crystal run missing_errnos.cr -- sample.txt
Showing last frame. Use --error-trace for full trace.
In errno_issue.cr:35:54
 35 | if ex.os_error == Errno::ENODATA || ex.os_error == Errno::ENOATTR
                                                         ^
Error: undefined constant Errno::ENOATTR
Did you mean 'Errno::ENOTTY'?

lib LibXAttr
  {% if flag?(:linux) %}
    fun getxattr(path : LibC::Char*, name : LibC::Char*, value : LibC::Char*, size : LibC::SizeT) : LibC::Int
  {% end %}
  {% if flag?(:darwin) %}
    fun getxattr(path : LibC::Char*, name : LibC::Char*, value : LibC::Char*, size : LibC::SizeT, position : LibC::UInt32T, options : LibC::Int) : LibC::Int
  {% end %}
end

def getxattr(path, key, value, size)
  {% if flag?(:linux) %}
    LibXAttr.getxattr(path, key, value, size)
  {% end %}
  {% if flag?(:darwin) %}
    LibXAttr.getxattr(path, key, value, size, 0, 0)
  {% end %}
end

def raise_error()
  raise IO::Error.from_errno("Xattr Error")
end

begin
  size = getxattr(ARGV[0], "user.nonexisting", nil, 0)
  raise_error() if size == -1

  ptr = Slice(LibC::Char).new(size)
  res = getxattr(ARGV[0], "user.nonexisting", ptr, size)
  raise_error() if res == -1

  puts String.new(ptr)
rescue ex: IO::Error
  if ex.os_error == Errno::ENODATA || ex.os_error == Errno::ENOATTR
    puts "Xattr not found"
  end
end

$ crystal -v
Crystal 0.35.1 (2020-06-19)

LLVM: 10.0.0
Default target: x86_64-apple-macosx

asterite commented 4 years ago

Can we make Errno all the definitions combined from all platforms? Maybe this is what @oprypin was suggesting. Then code is portable because definitions are the same for all platforms. Values might be different, but it doesn't matter because Errno values are never produced by a user, only consumed. Though one will have to use one definition or another in the case of this issue, but that's probably fine.

aravindavk commented 4 years ago

Can we make Errno all the definitions combined from all platforms? Maybe this is what @oprypin was suggesting. Then code is portable because definitions are the same for all platforms. Values might be different, but it doesn't matter because Errno values are never produced by a user, only consumed. Though one will have to use one definition or another in the case of this issue, but that's probably fine.

The code from src/file.cr is raising custom exception by internally handling the error numbers. But this again calling Errno::*

https://github.com/crystal-lang/crystal/blob/master/src/file/error.cr#L8-L19

private def self.new_from_errno(message, errno, **opts)
    case errno
    when Errno::ENOENT
      File::NotFoundError.new(message, **opts)
    when Errno::EEXIST
      File::AlreadyExistsError.new(message, **opts)
    when Errno::EACCES
      File::AccessDeniedError.new(message, **opts)
    else
      super message, errno, **opts
    end
  end

Some error numbers will have different meanings based on the context. All errors can't be generalized. ENODATA is used for different purposes in Mac/Darwin/BSD than on Linux. To write a generic exception for the above example, the code can be as follows.

{% if flag?(:linux) %}
no_xattr_error = Errno::ENODATA
{% end %}
{% if flag?(:darwin) %}
no_xattr_error = Errno::ENOATTR
{% end %}

private def self.new_from_errno(message, errno, **opts)
    case errno
    when Errno::ENOENT
      File::NotFoundError.new(message, **opts)
    when Errno::EEXIST
      File::AlreadyExistsError.new(message, **opts)
    when Errno::EACCES
      File::AccessDeniedError.new(message, **opts)
    when no_xattr_error
      File::NoXattrDataError.new(message, **opts)
    else
      super message, errno, **opts
    end
  end

Note: We need Errno for internal use to raise the exception. The PR will help even if anyone introduces the new Exception class.
Note 2: ENODATA may not be raised due to xattr not found, there may be other use cases. If we raise NoXattrData then it may add confusion. Similarly, ENOENT will be raised when no file/dir exists to which we are trying to do the IO, but when we try to create a file, ENOENT will be raised if the parent directory not exists.

$ python3 -c 'open("/tmp/nonexisting-dir/file1", "w").write("Hello")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/nonexisting-dir/file1'

asterite commented 4 years ago

@aravindavk does this mean you agree with the approach I suggested, or not?

aravindavk commented 4 years ago

@aravindavk does this mean you agree with the approach I suggested, or not?

Sorry if I added more confusion. Your suggestion looks good to hide the internal error codes from the user. But that is a new feature and not related to this issue.

I don't think I can spend enough time immediately to analyze all common Exceptions and merging the error codes. I feel the current patch is still useful even if anyone plans to implement the common exceptions(Because to raise new Exception, Errno enum is used).

aravindavk commented 4 years ago

Current PR is adding the missing error codes and doesn't change the existing behavior. (Refactoring helped to reduce some duplicate definitions but doesn't change the behavior)
Hiding Errno with custom Exception class is a new feature not related to this issue.

oprypin commented 4 years ago

@asterite https://github.com/crystal-lang/crystal/issues/9572#issuecomment-653755214 is correct. Sent that out as PR #9573

asterite commented 4 years ago

I wasn't talking about exceptions, I was just talking about Errno. Basically, if Errno defines A and B in linux, and B and C in Mac, let's define a single Errno that contains all of A, B and C. For Mac the value of A can just be -1 or some other value. It doesn't matter because that value doesn't exist there.

aravindavk commented 4 years ago

I wasn't talking about exceptions, I was just talking about Errno. Basically, if Errno defines A and B in linux, and B and C in Mac, let's define a single Errno that contains all of A, B and C. For Mac the value of A can just be -1 or some other value. It doesn't matter because that value doesn't exist there.

Makes sense. I can work on moving all the error codes to a single place, defining only error codes which are different in each platform.

PR from @oprypin is solving the problem in a different way(with error code duplication). Even with that PR, missing error codes need to be added in multiple files as required and also src/errno.cr.

Let me know if I continue to spend time moving error codes to single place with -1 for missing codes. Or @oprypin's approach to add -1

aravindavk commented 4 years ago

@asterite #9572 (comment) is correct. Sent that out as PR #9573

Still don't solve the docgen issue. Generated docs will show wrong values or -1 for some platforms.

oprypin commented 4 years ago

the docgen issue

Yes, that's a problem, though this is not the first time we run across a problem of this sort. Occasionally an idea pops up to not show enum values in the docs, but I don't think there is a dedicated issue/discussion for that.

I can work on moving all the error codes to a single place

I don't think that's the way to go.

Let me know if I continue to spend time

Maybe this just needs to wait on the discussion of #9573.

That said, I don't think your PR needs to be blocked on any of these decisions; I commented on https://github.com/crystal-lang/crystal/pull/9523#discussion_r449776789 with one possible solution.

crystal-lang / crystal

Missing a few LibC Error numbers #9572