-
Notifications
You must be signed in to change notification settings - Fork 226
Use flock() instead of UUCP-style locking for serial devices #1770
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
Open
taw10
wants to merge
46
commits into
OpenLightingProject:master
Choose a base branch
from
taw10:serial-flock
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
b142600
Use flock() instead of UUCP-style locking for serial devices
taw10 0556472
Ignore a codespell false positive due to line wrapping
peternewman dd34c1d
Always compile UUCP code
taw10 cf4be90
Rename UUCP lock routines
taw10 638daf7
Rename AcquireLockAndOpen -> AcquireLockAndOpenSerialPort
taw10 5d0cb39
Add ReleaseSerialPortLock (to match AcquireLockAndOpenSerialPort)
taw10 15a299b
Fix indentation
taw10 10e4ac6
OpenAndFlock: Return false if flock() is not available
taw10 d195639
Add a message when locking with flock()
taw10 ce8f9b4
Restore documentation of deprecated UUCP locking routines
taw10 d9f2721
OpenAndFlock: close FD if flock() is not available
taw10 4ea7de4
Add extra spaces before comments
taw10 6951d76
Wrap a long line
taw10 61752ab
Add some more debug messages
taw10 d45704f
Add deprecation dates
taw10 53b8c2c
Add parentheses to make Doxygen link a reference
taw10 9cd59a8
Add SerialLockTester
taw10 3e072f1
Don't fail if TIOCEXCL doesn't work
taw10 4bcf0f6
SerialLockTester: formatting fixes
taw10 d035bde
SerialLockTester: rearrange includes
taw10 379db11
SerialLockTester: Rearrange includes again
taw10 662b6e9
SerialLockTester: Create the test file
taw10 b394083
Update include/ola/io/Serial.h
taw10 8ca5f34
Remove pre-processor comment
taw10 11b2f40
Mention flock() when saying "No unlock necessary"
taw10 ff04282
Update include/ola/io/Serial.h
taw10 6138823
Update common/io/SerialLockTester.cpp
taw10 3d9ae1d
Fix debug message and over-long line
taw10 7fe0fa5
SerialLockTester: Eliminate unused variable
taw10 f9dec96
Split serial port locking into three separate steps
taw10 0bfe200
Formatting
taw10 9fe41fe
Fix include order
taw10 f60bd33
Clarify info message about TIOCEXCL
taw10 4806d31
Restore deleted comment
taw10 0dfa3f9
Add explanatory comment to #endif
taw10 f97bc8d
SerialLockTester: Add ".pid" to test filename
taw10 a528123
Update comments about serial port locking
taw10 98d4b66
Fix ordering of RemoveUUCPLockFile() and close()
taw10 64d4943
Add more #endif comments
taw10 3ada1bd
AcquireLockAndOpenSerialPort: Release UUCP lock on error paths
taw10 277e151
Merge branch 'master' into serial-flock
taw10 227a613
Apply suggestions from code review
taw10 185b881
Merge branch 'master' into serial-flock
taw10 a93bde9
Merge branch 'master' into serial-flock
taw10 3c2afc8
Merge branch 'master' into serial-flock
taw10 ae7de43
Merge branch 'master' into serial-flock
taw10 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,21 +60,27 @@ typedef enum { | |
| bool UIntToSpeedT(uint32_t value, speed_t *output); | ||
|
|
||
| /** | ||
| * @brief Try to open the path, respecting UUCP locking. | ||
| * @brief Try to open the path and obtain a lock to control access | ||
| * @param path the path to open | ||
| * @param oflag flags passed to open | ||
| * @param[out] fd a pointer to the fd which is returned. | ||
| * @returns true if the open succeeded, false otherwise. | ||
| * | ||
| * Depending on the compile-time configuration, this will use either flock() | ||
| * (the default) or UUCP locking. See: ./configure --enable-uucp-locking. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this now doing both/all? |
||
| * | ||
| * This fails-fast, it we can't get the lock immediately, we'll return false. | ||
| */ | ||
| bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd); | ||
| bool AcquireLockAndOpen(const std::string &path, int oflag, int *fd); | ||
|
peternewman marked this conversation as resolved.
Outdated
|
||
|
|
||
| /** | ||
| * @brief Remove a UUCP lock file for the device. | ||
| * @param path The path to unlock. | ||
| * | ||
| * The lock is only removed if the PID matches. | ||
| * | ||
| * Does nothing if UUCP locking is not in use | ||
| * (see ./configure --enable-uucp-locking). | ||
| */ | ||
| void ReleaseUUCPLock(const std::string &path); | ||
| } // namespace io | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Has sys/file.h existed forever, so we're not breaking UUCP builds which don't have it?
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.
I'm not sure, I'll investigate. However the header is checked by the configure script, and used without any #ifdefs in one other place (KarateLight).
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.
Ah okay, obviously that plugin breaking wouldn't have quite so much impact, but the plugin has been around for a while and no-one has complained so we're probably good...
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.
he flock call itself conforms to 4.4BSD, i.e. it's been around since 1997. So I think we're pretty safe here.
I could add guards around the #include (ifdef HAVE_SYS_FILE_H), but if it's worked so far then I don't see any reason to add the extra complexity. On inspection, I'm not actually sure why KarateLight includes this header at all.
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.
Yeah very good point about the fact it's worked!
I'm not sure about KarateLight either; @cpresser ?
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.
Unfortunately, I am not sure what to answer here. My C++ is quite rusty, I have not used it in years and sure have no idea which include contains which functionality.
I recall that I did include file locking of the serial-port-device in the karatelight code. A quick search turned up this piece of code here:
https://github.com/OpenLightingProject/ola/blob/master/plugins/karate/KarateLight.cpp#L121
Does that help?
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.
The
#include <sys/file.h>has been there since the first version of KarateDevice.cpp (f69a26b), but theflockcall is in a different file (KarateLight.cpp). I suspect theflockcalls were originally in KarateDevice.cpp and got moved.It does mean, however, that there's not only the #include but also a real use of flock() in OLA already without any tests! Therefore we're probably pretty safe using it for all serial devices. Nevertheless, we'll keep the tests and the conditionals :)