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

getpw* and getgr* functions no longer raise Not_found on unix errors #1451

Merged
merged 5 commits into from Dec 15, 2017

Conversation

Projects
None yet
6 participants
@aalekseyev
Copy link
Contributor

aalekseyev commented Oct 26, 2017

This change makes functions getpwuid, getgrgid, getpwnam, getgrnam only raise Not_found when the unix call does keep the old value of errno (therefore indicating success).

If it changes errno then we raise the appropriate unix error.

Tested this by emptying /etc and using one of those functions. Got:

Fatal error: exception Unix.Unix_error(Unix.ENOENT, "getpwnam", "")

aalekseyev added some commits Oct 26, 2017

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Oct 27, 2017

This is ok. Please update the entry in the Changes file to add the author and reviewer.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Oct 27, 2017

Quoting from the Linux man page:

ERRORS
       0 or ENOENT or ESRCH or EBADF or EPERM or ...
              The given name or uid was not found.

Why would you raise Not_found in the 0 case and a Unix_error in the ENOENT, etc, cases?

How much breakage in existing code will that cause?

@aalekseyev

This comment has been minimized.

Copy link
Contributor Author

aalekseyev commented Oct 27, 2017

I think the piece you quoted is the man page's way of saying that errno stays unchanged (therefore "..." since it has no control over what the result will be).
Oh, that's not right. It did raise ENOENT in my test, which is only listed under "name was not found" section.

The reason to raise Not_found only when errno stays unchanged is this paragraph:

The getpwnam() and getpwuid() functions return a pointer to a passwd structure,
or NULL if the matching entry is not found or an error occurs.
If an error occurs, errno is set appropriately. If one wants to check
errno after the call, it should be set to zero before the call.

I expect the breakage to be small: this is both an unlikely failure case and and unlikely to be distinguished by users (often both errors would just be fatal). In the rare case where the user cares about whether it's Not_found or another exception, I expect this change to be an improvement for them.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Oct 27, 2017

The reason to raise Not_found only when errno stays unchanged is this paragraph:

This seems consistent with POSIX.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Oct 27, 2017

For the record, I approved this because I think the proposed code structure is the de facto way of distinguishing the error return cases for such functions (as the page @dbuenzli quotes says).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Oct 27, 2017

The reason to raise Not_found only when errno stays unchanged is this paragraph:

I read this paragraph too. My point is that even when errno is changed to, say, ENOENT, raising Not_found may be the correct thing to do.

Again: I'm afraid of the breakage this will cause with C libraries that are not quite POSIX and will occasionally set errno for a situation that is best described as Not_found (an error that the user expects and can handle) rather than Unix_error (an error that often aborts the program).

@aalekseyev

This comment has been minimized.

Copy link
Contributor Author

aalekseyev commented Oct 27, 2017

I don't think I can estimate breakage well, but one way to reduce potential breakage is to just raise unix error on EIO, EMFILE, ENFILE, ENOMEM (not in posix?), ERANGE, EINTR (this is maybe the one that motivates this PR).

Would you prefer that, or do we need some sort of breakage investigation?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Oct 27, 2017

I would prefer to leave the code as it is today. Trying to make a distinction between "normal" (Not_found) and "abnormal" (Unix_error) failures for this set of functions is not obvious (where to draw the line?) and not very useful anyway.

@aalekseyev

This comment has been minimized.

Copy link
Contributor Author

aalekseyev commented Oct 27, 2017

I would argue EINTR is important to raise as unix error because in that case the correct application behavior is retry. The others I care much less about.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Oct 27, 2017

I would argue EINTR is important to raise as unix error because in that case the correct behavior is retry.

That's fair. I can agree with that. Would you change this pull request accordingly?

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Oct 27, 2017

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Thanks for the changes. This looks good to me.

@xavierleroy xavierleroy merged commit 5ec0218 into ocaml:trunk Dec 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.