Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

core: Make errno message a part of errno exceptions #2695

Closed
wants to merge 10 commits into from

Conversation

AndreasMadsen
Copy link
Member

To people like me there don't use enough time with errno codes it can be difficult to remember what EBADF means. This add a text message to all errno exceptions, like bad file descriptor.

The messages are from get_uv_errno_message, to use this it was necessary to change the SetError so it set errno to a number and not get_uv_errno_string.

I'm hoping we later could abstract ErrnoException from node.cc to use Errno::ErrnoException.

It would also be nice to have binding functions return an uv_errno number instead of 1 or 0. This way we won't pollute global scope with errno.

Example of this patch:

var Pipe = process.binding('pipe_wrap').Pipe;
var errnoException = process.binding('errno').errnoException;

var stream = new Pipe(true);
    stream.close();

var buffer = new Buffer("test");
var errno = stream.write(buffer, 0, buffer.length);
if (errno !== 0) {
  throw errnoException(errno, 'write');
}

@piscisaureus told me that you did not want any errno changes now, but since I had made the patch why not allow you to reconsider.

@bnoordhuis
Copy link
Member

It would also be nice to have binding functions return an uv_errno number instead of 1 or 0. This way we won't pollute global scope with errno. Example of this:

Agreed. We could (and probably should) copy the Linux kernel convention here: return >=0 on succes, -ERRNUM on error.

@Nodejs-Jenkins
Copy link

Can one of the admins verify this patch?

@tjfontaine
Copy link

This work has been done in libuv and in node, in various commits. libuv now directly returns the errno and we map that to a useful value and include the syscall

@tjfontaine tjfontaine closed this Feb 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants