ports: Fix isatty syscall error returning#5
Conversation
| + if (!sys_ioctl(fd, TIOCGWINSZ, &ws, nullptr)) | ||
| + return 0; | ||
| + if (auto err = sys_ioctl(fd, TIOCGWINSZ, &ws, nullptr)) | ||
| + return err; |
There was a problem hiding this comment.
I think this should check whether the error returned by ioctl was EBADFD or something else, in which case it should return ENOTTY. The ioctl for most files will return either ENOSYS or EINVAL, which is not what POSIX dictates for isatty
There was a problem hiding this comment.
I briefly considered that too when I opened the PR, but as long as ioctl TIOCGWINSZ is working properly, it shouldn’t return ENOSYS or EINVAL. I suppose there is a slight chance EFAULT might occur if the process stack is corrupted somehow, but at that point, the calling process has more things to worry about than the return value of isatty.
I can add an assertion that the error returned by ioctl is either ENOTTY or EBADF if you want, but other than that I’d argue that returning ENOTTY if the error is anything other than EBADF just ignores the fact that something terribly wrong (like TIOCGWINSZ being broken) is happening if it’s returning something like EINVAL.
There was a problem hiding this comment.
TIOCGWINSZ is an ioctl that's handled only by a TTY, ioctl on a file will not return ENOTTY
There was a problem hiding this comment.
Huh? Doesn’t ioctl TIOCGWINSZ return ENOTTY if fd is a valid file descriptor that doesn’t point to a TTY?
https://man7.org/linux/man-pages/man2/ioctl.2.html
https://linux.die.net/man/2/ioctl
There was a problem hiding this comment.
Not sure, but even if that's the case then my VFS needs a bit of rework since I forward ioctl calls to VFS nodes. ioctl can return ENOSYS only if the node doesn't provide an ioctl interface, otherwise it is up to the node to handle the request. Like here for example: https://github.com/czapek1337/zigux/blob/8441f3bb3bbfdca35441c0d0223661436ac5a574/src/vfs/dev_fs.zig#L299-L345
There was a problem hiding this comment.
Hmm, I see. In Tonix’s VFS each vnode has a VnodeType enum that can be VnodeType::Terminal and it just returns ENOTTY if it isn’t, but that’s kind of a clumsy way to do it. I guess we’re postponing this PR until zigux’s ioctl is fixed, then.
2 participants
This PR fixes the issue where the
isattysyscall was never returningEBADF, even whenfdwas an invalid file descriptor.Resolves #4.