14 25 sysfs permissions #26
Conversation
gpio/__init__.py
Outdated
| f.write(str(self.pin)) | ||
| f.flush() | ||
| # give the kernel a moment to build out the requested sysfs tree | ||
| sleep(.1) |
There was a problem hiding this comment.
This is what flush was supposed to do.
If this is happening, then a sleep here will only make this flaky -- there will still be cases where failure happens, they just won't show up in tests as often (which is probably even worse).
There was a problem hiding this comment.
Looping with os.path.exists() would likely be a better fix.
It would be, however it can also run into a race condition if another process deletes it before we see it. It's definitely better than a sleep though.
There was a problem hiding this comment.
So it turns out the value file was being created immediately on my system. But it is initially created owned by root:root. If you are running as root, then this is why you didn't get errors here. Unfortunately, running as root is not an option in my environment. I believe udev comes along and changes the file to root:gpio (on stock Raspberry Pi OS), but this change takes a few ms. The latest commit watches for this change and is able to continue after a 30ms delay on my Pi 4. The half second timeout should prevent deadlock on race condition.
There was a problem hiding this comment.
Well, that helps explain it.
I'm not sure how best to support this. I'm pretty sure that a sleep is not the right approach :D
What we want: a boolean flag which loops, waiting for the right condition. That condition would wait until...the file ownership changes?
But that likely won't work for everyone. Which owner should we wait for? The default of "current user" should work most of the time, but will it always work?
We could allow you to pass in a function... which we call at the right moment. But that's extremely messy as well.
I think probably we messed up on the API a bit. The constructor should not be performing logic. If we had a staticmethod constructor for the default case and customized constructors for more specialized cases, then we could break up this logic more easily.
However, it's all up to @Gadgetoid as I am no longer the lead maintainer, and that would be a breaking change. Just some thoughts.
There was a problem hiding this comment.
9991c3 waits for the group owner to change, matching that of the /sys/class/gpio/ directory. This should work for everyone. Since it gives up waiting after 0.5 seconds, there should be no breaking change here.
There was a problem hiding this comment.
Sorry I can't believe how far behind the curve I am with this. I've been working on getting libgpiod up to scratch, among other things.
This PR needs split up into two individual pulls. One to fix the file modes, which are a very imminent and clear cut problem (well certainly as of a year ago, sigh) and one to address the ownership.
IMO it's my preference simply to duck-type ownership - rather than checking the group we should just try to open the device, catch the error and re-try a number of times before raising an exception to the user.
I'm not averse to changing the API if we need to. I'd rather have a good library than worry about people having to make downstream code changes.
There was a problem hiding this comment.
IMO it's my preference simply to duck-type ownership - rather than checking the group we should just try to open the device, catch the error and re-try a number of times before raising an exception to the user.
Disagree with auto-retry -- that is something for a client (with a retry library) to do, not us.
I'd be okay with having a paramater or default retry function to help folks have an a simple and easy path.
There was a problem hiding this comment.
Not sure I agree there- if it’s common behaviour for the system to be slow to assign relevant permissions to a file node we’re explicitly expecting to be writable, then that’s an implementation detail and not something to be surfaced to the user. Particularly as opening a pin - which necessarily requires an export - already has side-effects that could leave the gpio in an unexpected state.
You could argue for a change of API that requires the user to set up the initial value and direction outside the constructor, but that’s kicking the can down the road somewhat- if the user has to care about implementation details and minor per platform caveats, what’s the library even for?
There was a problem hiding this comment.
I guess I can see the point of keeping things simple and slow. Sorry, I deal with lots of "async" design paths in my work and was probably over-applying the best practices.
|
Okay I have added the filemode patches to #31 and https://github.com/vitiral/gpio/tree/python-2.7 In lieu of sleep or waiting on a specific gid, I think we should - as mentioned above - just retry with a delay, and fail after N retries. I think that's likely to work reasonably in all cases and avoid edge cases and regressions. |
| value_path = os.path.join(self.root, 'value') | ||
| start = time() | ||
| # give udev a moment to update the group of newly created files | ||
| while os.stat(value_path).st_gid != gpio_gid and time() - start < .5: |
There was a problem hiding this comment.
should you throw some kind of error if this ISN'T true after the loop?
Fixes #14 and #25 by adding file modes which better match sysfs file permissions
Fixes subsequent permission error when export is successful but sysfs tree buildout is not complete before attempting to use the new value FH. Slower systems may need a longer delay. Looping with os.path.exists() would likely be a better fix.