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

Add posix_errno to H5E_error2_t struct #34

Closed
wants to merge 1 commit into from

Conversation

takluyver
Copy link
Contributor

This is my patch from HDFFV-10631. I'm interested to see if the CI passes. :-)

@takluyver
Copy link
Contributor Author

Just to reiterate what I said about the patch: I'm not much of a C programmer, so this may be unusable. My main aim is to illustrate why I think this is not too big a change.

I haven't worked yet on integrating this with h5py; I hope to look into that at some point when I get time.

@qkoziol
Copy link
Contributor

qkoziol commented Oct 14, 2020

Hi Thomas!
This would be a modification to the public interface, so it needs a high degree of thought, and if accepted, would only be possible to deploy in a future "major" release (i.e. 1.14.0). With that in mind, I looked at your code and think that expanded all the internal calls to push errors on an error stack with a '0' POSIX errno would not be a great direction to go. What do you think about pushing another error on the error stack, like the MPI errors, with the string from strerror() as the description? That would allow the POSIX errno to be only captured and maintained for POSIX calls and would avoid modifying the public API also.

Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to pushing another error on the error stack, using strerror(), instead of expanding the parameters for pushing errors.

@takluyver
Copy link
Contributor Author

I'm not familiar with the error handling for MPI - is there a particular bit of code you can point me to?

My interest in this is in having the errno itself available in a machine-readable way, to allow more consistent exceptions from h5py. I'm not sure there is a backwards-compatible way to do that, except by scraping it from the string description (errno = %d) - we've asked about that previously, and been told that we shouldn't rely on it.

@qkoziol
Copy link
Contributor

qkoziol commented Oct 14, 2020 via email

@takluyver
Copy link
Contributor Author

Thanks, I've found the macros you mentioned.

If string scraping is going to be the answer anyway, is there any chance of declaring that the errno = %d already in the description for these errors is reliable? It's been consistent since you wrote it 11 years ago (a443284), so we could instantly support it in existing versions of HDF5. The only reason we're not already doing that is that we were told the description can't be relied on - which I understand as a general policy, but it would be really convenient to rely on this specific detail. @tacaswell relayed that to me; I'm not sure who he asked.

@qkoziol
Copy link
Contributor

qkoziol commented Oct 14, 2020

Well, true, but I think adding the output from strerror() is valuable also. And that description string would be a standardized place that we could guarantee the errno scraping didn't change.

@takluyver
Copy link
Contributor Author

The strerror is already there as well 😉

@qkoziol
Copy link
Contributor

qkoziol commented Oct 14, 2020

Ah - missed that earlier. :-) Seems like you should be OK then - maybe just a comment in the HSYS* error macros that h5py (and maybe other layers) is depending on the output format, so it doesn't accidentally get changed in the future?

@tacaswell
Copy link

My memory of walking through this was in the context of h5py/h5py#1099

@takluyver
Copy link
Contributor Author

OK, I've opened a new PR to add the comment: #37. We'll look at using this after releasing h5py 3.0. Thanks!

@takluyver takluyver closed this Oct 15, 2020
vchoi-hdfgroup added a commit that referenced this pull request Aug 12, 2021
@bmribler bmribler mentioned this pull request Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants