Make serial_logger easier to work with
#375
Open
Conversation
This file contains 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
Requiring a program to run as root is a big hammer to an otherwise
delicate situation, and is sure to get a UNIX engineer's blood
boiling with instant rapidity.
For the case of serial_logger, permissions are required for opening
the serial port block device. When those permissions are not present,
the program exits gracefully:
$ sudo -u nobody ./release/serial_logger /dev/ttyUSB0
AqualinkD serial_logger V2.7
Error: RS Serial: Unable to open port: /dev/ttyUSB0, error 13
Error: Serial Log:Unable to open port: /dev/ttyUSB0
Permission denied: /dev/ttyUSB0
However, the program can be run successfully without root. In most
distros, this is achieved with the **dialout** group. As long as the
user is a member of **dialout**, things will run as expected:
$ ls -l /dev/ttyUSB0
crw-rw----+ 1 root **dialout** 188, 0 Jan 2 13:04 /dev/ttyUSB0
$ groups
mrnuke wheel **dialout** lock
$ ./release/serial_logger /dev/ttyUSB0 -d
AqualinkD serial_logger V2.7
Debug: RS Serial: Openeded serial port /dev/ttyUSB0
Notice: RS Serial: Port /dev/ttyUSB0 low latency mode is set
Info: RS Serial: Port /dev/ttyUSB0 set I/O blocking attributes
Remove the pointless root check. For users that already run as root,
nothing changes. Users that prefer to use granular more permissions
can now run without requiring root.
It appears that `intHandler()` is designed to terminate the main loop
by setting the `_keepRunning` flag. However, this logic is defective,
as it assumes that the main loop will terminate within a reasonable
time. This is not the case:
(gdb) start
(gdb) continue
<CTRL-C>
(gdb) signal SIGINT
Continuing with signal SIGINT.
Notice: Serial Log:Stopping!
<CTRL-C>
(gdb) backtrace
0 0x00007ffff7cea121 in read () from /lib64/libc.so.6
1 0x0000000000407002 in get_packet ()
2 0x00000000004038b1 in _serial_logger ()
3 0x00000000004026ed in main ()
The main loop gets stuck in a blocking call to `read()`. If there is
no data coming via the serial port, `read()` never returns, and the
porogram never exits.
This behavior is quite problematic, as, intuitively, CTRL-C
terminates most programs by sending SIGINT. To stop `serial_logger`,
one generally needs to freeze the program with CTRL-Z, and then
send SIGKILL to terminate it. I presume this is not the intended
behavior of the main loopp.
Fixing this behavior would require re-architecting the main loop to
use asynchronous or non-blocking calls. For the short term, simply
exit on SIGINT and SIGTERM.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Don't force
serial_loggerto run as root. That's very bad (TM). Also exit immediately upon SIGINT.More details in the individual commit messages