-
Notifications
You must be signed in to change notification settings - Fork 91
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
handle UnsatisfiedLinkError in LinuxPOSIX constructor, resolves #187 #188
Conversation
@betalb So I have a question which you may know. The original code is written around each of these three foreign calls potentially being independently defined (lazily setting bool for each one to fall back when it is wrong). I would think typically it would be all or nothing (like your PR does) but it isn't how the rest of this code is written. The original statVersion test is an all or nothing field but the check only checks one stat. If we could assume alll 3 calls are always defined or not defined we could get rid of all this try/catch stuff in the individual stat methods. I think your PR looks ok to me and it definitely fixes an obvious problem on systems missing these calls. @headius do you have any thoughts on this? I half think we should assume all or nothing based on presence of xstat64 and then remove the volatile booleans and the extra logic in each stat. |
I should add this appears to be written around lazily working but it clearly only worked lazily ONLY is __xstat64 worked. So at a minimum we could eliminate that one from having volatile field and lazy logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but we might as well clean up the three booleans and move the whitespace cleanup to a separate commit.
@betalb could you change this to be a single final boolean for all 3? Since this happens in the constructor and we try once we can then remove all UnsatisfiedLinkErrors in the 3 stat methods and only check that boolean value. If you don't want to we can do it but it might be fun to delete code? 😄 |
75bb232
to
bdd587f
Compare
I did the changes, discussed here I can make another change and move error reporting to method versions with 2 arguments (int fd, FileStat stat). Though I see that super-classes follow the same pattern and report error in stat versions that accept only integer argument and return stat object. |
@betalb There's definitely an unfortunate lack of consistency in these APIs, as we have been slowly adding and improving them over the past 10-15 years. We would definitely welcome any additional PRs that make things more consistent! I will review the updated PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner this way! Thank you for making the updates.
POC PR that will resolve #187