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

Correct And Simplify Memory Handling in xattr_get #17

Closed
ldo opened this issue May 7, 2018 · 4 comments
Closed

Correct And Simplify Memory Handling in xattr_get #17

ldo opened this issue May 7, 2018 · 4 comments

Comments

@ldo
Copy link

ldo commented May 7, 2018

I notice your xattr_get routine is missing a check for an error from the PyList_Append call.

While I was at it, I took the opportunity to rework the xattr_get logic to reduce the complexity of the control paths, eliminating all the gotos. As you can see from the enclosed patch, it becomes much easier to verify that everything will be be correctly disposed regardless of what errors might occur.

ldo-xattr_get.patch.txt

@ldo
Copy link
Author

ldo commented May 7, 2018

Sorry, noticed an omission in the patch. Update enclosed.

ldo-xattr_get.patch.txt

@iustin
Copy link
Owner

iustin commented Nov 17, 2019

Hi,

Refactoring the way error handling is dome is, I consider, more a matter of taste. I'd like to discuss that separately from fixing an actual bug. Can you rebase and split the patch into bug-fix and rework-error-handling?

@ldo
Copy link
Author

ldo commented Nov 19, 2019

Code quality has nothing to do with “taste” (whatever that might mean).

@iustin
Copy link
Owner

iustin commented Nov 26, 2019

Error handling methods, as long as correct, are a matter of style, and not of "quality". I'll fix the PyList_Append error checking, if you do want to send a pull request for the refactoring, please do that separately.

iustin added a commit that referenced this issue Nov 26, 2019
This fixes the correctness aspect of #17, although not the entire
point.
@iustin iustin closed this as completed Nov 26, 2019
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

No branches or pull requests

2 participants