Zig: Should EINVAL always be unreachable?

[jorangreef]

The std lib handles EINVAL as unreachable for os.openatZ().

However, there are some legitimate runtime use cases that could trigger EINVAL, e.g. trying to open a file descriptor in O_DIRECT on Linux, but on a file system that does not support O_DIRECT.

This then leads to undefined behavior rather than a safe error.

It also means you would have to reimplement os.openatZ() if you wanted to write something to detect fs support for O_DIRECT.

There are also other scenarios that could trigger EINVAL, e.g. the system does not support synchronized I/O for this file, or the O_XATTR flag was supplied and the underlying file system does not support extended file attributes.

These are all fairly common failure modes for storage/backup/sync applications that need to probe file system characteristics.

Should EINVAL always be unreachable? Could the std lib start to move away from unreachable for this and other syscalls?

[jorangreef]

Related: https://github.com/ziglang/zig/issues/6389

[andrewrk]

It should be handled on a case-by-case basis. For some syscalls, EINVAL should be unreachable because it can only mean something like an invalid pointer address being passed to the kernel, or invalid flags.

However, as you pointed out, some kernel APIs unfortunately have decided to dual-purpose this error code to mean other things, in which case we must to handle the ambiguity by mapping it to an error code. We had to make this change with EBADF recently for some syscalls, for example.

I do want to keep the error sets clean, however, so I am opposed to a blanket modification of all EINVAL code sites without cause.

[jorangreef]

For some syscalls, EINVAL should be unreachable because it can only mean something like an invalid pointer address being passed to the kernel, or invalid flags.

Ah I see... so returning an error would be less safe in that particular case compared to unreachable (or panic) because the program state is likely corrupted. If the intention is to crash the program and fail fast, would there be a reason not to use @panic?

[JOT85]

@jorangreef @andrewrk I recently I came across this issue. I was writing a script to do a few things, including setting my screen brightness. I'm on an AMD system so I'm writing to /sys/class/backlight/amdgpu_bl0/brightness. If you write an invalid number, or a number that's out of range, it gives error EINVAL, however in std/os.zig, pub fn write, it marks EINVAL as unreachable so I can't handle this error nicely.

The man page describes EINVAL as:

fd is attached to an object which is unsuitable for writing; or the file was opened with the O_DIRECT flag, and either the address specified in buf, the value specified in count, or the current file offset is not suitably aligned.`

So I think this is mainly the fault of my amdgpu driver misbehaving, but I feel like this kind of error, which is handed to the program, should be returned.

I do want to keep the error sets clean, however, so I am opposed to a blanket modification of all EINVAL code sites without cause.

Maybe adding EINVAL to the error sets isn't the right solution. I think the errors currently marked as unreachable should return unexpected since that error result is unexpected (it shouldn't happen) but not necessarily unreachable (because it can occur, either with a driver like amdgpu, or, as #6389 mentions, changing kernel behaviour).

unreachable isn't a great way to behave upon something that can, and does, actually happen.

[andrewrk]

If the intention is to crash the program and fail fast, would there be a reason not to use @panic?

Yes. @panic is what unreachable does in safe build modes. In unsafe build modes, it is undefined behavior. unreachable is semantically correct, and the application developer has the knob to decide how to exploit the semantics: allow the compiler to optimize unreachable, or turn them all into panicks. In a future improvement to zig, the application developer will have this knob at a finer grained level, applying different release modes to different packages.

However what we can do is decide that the code path is not, in fact, unreachable, and that leads us to...

I think the errors currently marked as unreachable should return unexpected since that error result is _unexpected_ (it shouldn't happen) but not necessarily _unreachable_ (because it can occur, either with a driver like amdgpu, or, as #6389 mentions, changing kernel behaviour).

I would support a change to make EINVAL map to error.Unexpected for the functions where we already have Unexpected in the error set. Note that what we are doing here is saying, this code path is not unreachable, but it is unexpected.

We may want to consider doing this carefully - in a debug build currently, hitting that EINVAL => unreachable line is a great debugging experience, and returning error.Unexpected would be slightly less ideal.

[jorangreef]

We may want to consider doing this carefully - in a debug build currently, hitting that EINVAL => unreachable line is a great debugging experience, and returning error.Unexpected would be slightly less ideal.

Agreed, I would just add that even in debug builds, there are situations where an error should be thrown for EINVAL, and where this would be the right thing to do e.g. where you need to test a filesystem for O_DIRECT support. Receiving and being able to handle EINVAL in this context is perfectly normal and expected as we would expect some Linux filesystems not to support O_DIRECT e.g. in some virtualized environments. There's no way to do this without having access to EINVAL as an error, and not being able to do this in debug builds would force the developer to resort to safe or fast optimizations, or writing their own syscall wrappers.

[marler8997]

@jorangreef also note that it's pretty easy to call a system call directly in Zig. If you're doing a platform-specific test like testing for O_DIRECT, it's easy to just call std.os.linux.open directly.

[jorangreef]

Thanks @marler8997 , yes we're using os.system.openat already but it would be great to use more of the std lib. O_DIRECT is more than platform-specific, it's also file-system specific (even on Linux).