clang-tidy
devscripts (includes ‘debuild’ script)
bear
- Also visit similar section in similar file in unixcw project.
- Test a cwdaewmon process that is running as system service
cwdaemon is installed on target OS and is executed as system daemon. This means that it may behave differently than when started by hand from build directory.
One example of a different behaviour is accessing PulseAudio: cwdaemon started by hand by developer has no problems with opening PulseAudio sink, but when started as system service by systemd, it can’t open PulseAudio sink.
You SHOULD use “command line”, You MUST NOT use “commandline”. You MAY use “command-line”.
You SHOULD start debug messages with capital letters. Example (“Invalid” is the first word in the message): “[ERROR] cwdaemon: Invalid value of option for keying device (expected ‘key|ptt=RTS|DTR|none’): [ke=none]”
- Use “option” when referring to command line tokens passed to cwdaemon
program that are used to configure and control the behaviour of the daemon.
Don’t use “command”. Don’t use “switch”. “Don’t use arg”.
“command” can be used in “command line option” phrase, but only as part of “command line”.
“–help” token is an option (a command line option). “-p 4242” is an option with value “4242”.
”./src/cwdaemon -n” starts a program with “-n” command line option.
Rationale: “man 1/3 getopt” talks about parsing of “options”.
- Use “reply” or “socket reply” when referring to data sent back by cwdaemon
server to clients over socket network.
The reply is sent back in response to either <ESC>h request or to caret request.
- Use “full reply” or “full socket reply” when referring to a reply that:
- includes leading ‘h’ character and trailing “\r\n” bytes and is sent in response to <ESC>H request
- includes trailing “\r\n” bytes and is sent in response to caret request.
- Use “code” or “Escape code” when describing a character that follows ESC character in Escape requests. This is the second character in the Escape requests.
- An USB dongle used to emulate UART device is an USB-to-UART device.
Good: USB-to-UART Not-so-good: USB-UART, USB/UART Bad: USB/tty, USB-serial, USB-to-tty, USB-COM
Rationale:
- CP2102 chip’s data sheet calls the chip a “USB-to-UART Bridge Controller”.
- FT232R chip’s data sheet calls the chip a “USB UART I.C.”.
- Request types
Messages sent to cwdaemon server can be of three types:
- plain request - contains just text to be played,
- caret request - like plain request, but the last character is a caret (‘^’), which triggers special behaviour in cwdaemon server,
- Escape request - request in which first character is an Escape character, the second character is an Escape request code, and the remainder of request is optional data.
- Capitalize “Escape” word in “Escape request” phrases.
Good: “send Escape request” Bad: “send escape request”
- Type of request SHOULD be in ALL CAPS:
Good: CWDEVICE Escape request Bad: cwdevice Escape request
Good: PLAIN request Bad: plain request
Good: CARET request Bad: Caret request
Summary: cwdaemon started by systemd can’t open PulseAudio sound sink.
Reproduction steps:
- Build and install libcw 7.0,
- Build and install cwdaemon 0.11.0 or 0.12.0
- Modify operating systems’s init scripts to start the cwdaemon
- Modify /etc/default/cwdaemon to use PulseAudio
- systemctl start cwdaemon
If you set sound system in cwdaemon’s config to “p” (PulseAudio) then cwdaemon/libcw will have problems opening the sound sink.
You can also see this when you start “cwdaemon -n -x p” by hand as root.
This is probably a bug in libcw, but I’m putting it here because it was found during work on cwdaemon and I didn’t investigate it in libcw yet.
The dir in deb package contains Makefile.am and Makefile.in, which are pretty useless for end-user. It would be better to have just a simple Makefile in that location.
Print XDG_RUNTIME_DIR, LD_LIBRARY_PATH and perhaps something else, but only if you enable it explicitly in code (#ifdef 0 by default).
This feature may help in debugging different problems.
I had to modify test code (extend env table of process) in order to avoid problems with PulseAudio.
Look at possibility to re-define pin assignments. Search for “cwdaemon suggestion” e-mail from Herman Tibor HA4TI https://forums.qrz.com/index.php?threads/cw-keyer-with-no-dtr-pin-cwdaemon-cwlib.744068/#post-5732680
Done for tty devices through -o/–option command line option.
Per man page on linux the function is removed in newer POSIX. Replace it with nanosleep().
Today (2024.01.06) the C code doesn’t use usleep() anymore. Sleep is done using nanosleep() - see src/sleep.c.
During execution of cwtest_escd.pl test, when invalid values are sent in escaped request, cwdaemon prints error log twice:
[ERROR] cwdaemon: invalid requested PTT delay [ms]: “0.096100” (should be integer between 0 and 50 inclusive) [ERROR] cwdaemon: invalid requested PTT delay [ms]: “0.096100” (should be integer between 0 and 50 inclusive)
cwdaemon_params_pttdelay() can return 0/1/2. Replace the integer values with enums.
Some functional tests implemented in Perl are sending float values as invalid values of escaped requests. Currently the values look like this:
Trying to set positive float value 0.010000 Trying to set positive float value 0.031000 Trying to set positive float value 0.096100 Trying to set positive float value 0.297910 Trying to set positive float value 0.923521 Trying to set positive float value 2.862915 Trying to set positive float value 8.875037 Trying to set positive float value 27.512614 Trying to set positive float value 85.289104 Trying to set positive float value 264.396222 Trying to set positive float value 819.628287 Trying to set positive float value 2540.847690 Trying to set positive float value 7876.627838
Testing cwdaemon with both 0.031000 and 0.096100, or with 264.396222 and 819.628287 doesn’t bring much value. The set of values should be re-evaluated.
Execution time of each functional test (and in future of non-functional test) should be:
- Measured during execution of a test binary (e.g. by subtracting uptime at end from uptime at start); the value should be displayed at the end of test.
- The value should be also recorded in qa/tests.org, so that it’s possible to estimate duration of functional tests in total.
- The value should be then also displayed at the beginning of each functional test, to give tester some expectation for duration of test.
Each test case in each type of test (functional, non-functional, unit test) should clearly and explicitly indicate PASS or FAIL result in output printed to console.
Tester should always be unambiguously informed about PASS/FAIL result. He should not be forced to read sentences in logs, he should be able to evaluate tests’ results just by looking at PASS/FAIL indicator in output of logs.
IN-PROGRESS: some of test do this, but probably not all yet.
Handling of the short and long options is duplicated between cwdaemon_args_process_short() and cwdaemon_args_process_long().
The unification has already started for “-o”/”–options” command line option: you can see in cwdaemon_args_long[] that the fourth field of ‘struct option’ is set to ‘o’, and that the option is handled by call to cwdaemon_params_options() only in cwdaemon_args_process_short().
Do this slowly, one option at a time. The processing of command-line options functions correctly so don’t try to adjust everything at once and accidentally break something.
Add/improve tools and procedures for static code analysis with clang-tidy.
IN-PROGRESS: there is ROOT/.clang-tidy and ROOT/qa/lint_clang_tidy.sh. The next step is to decrease count of exclusions in .clang-tidy.
Add/improve tools and procedures for static code analysis with cppcheck.
Add/improve tools and procedures for static code analysis with gcc fanalyse.
Code from src/log.c should be refactored to provide the following features:
- there should be single function or an unified and consistent set of
functions used for logging information. Currently three are three
functions:
- log_message()
- cwdaemon_errmsg()
- cwdaemon_debug()
- it should be possible to disable logging at compile time in a way that removes log message strings from cwdaemon binary. The purpose of this is to have an option to reduce size of cwdaemon binary.
- verbosity/severity of log messages doesn’t use custom VERBOSITY enums, but relies on LOG_ERR and friends.
IN-PROGRESS: log.h has new set of logging macros that are slowly being used in code base. They can be re-defined to be empty statements if necessary.
IN-PROGRESS: the new logging macros use a single function underneath that uses standard priority names from syslog.h.
IN-PROGRESS: the macros are introduced in new code or code being modified.
The macros from src/log.h are: log_error() log_warning() log_info() log_debug()
Write proper unit tests of cwdaemon_recvfrom(), including mocking of libc’s recvfrom().
Find next functions (existing or to-be-written) in cwdaemon that should be unit-tested.
Ticket R0016 is for specific function that needs special attention. Ticket R0017 is for unit testing in general.
Review a “TODO acerion 2024.03.17” comment added to ttys_init(). Evaluate how cwdevice::init() and cwdevice::free() should be called in cwdaemon_cwdevice_set() to properly de-init old device and to init new device.
Use the following code to trigger a valgrind error shown below:
#!/bin/bash
valid=”ttyUSB0” invalid=”hello”
declare -a commands=(“\x1b8/dev/”$valid “\x1b8/dev/”$invalid “\x1b8/dev/”$valid “\x1b8/dev/”$valid “\x1b8/dev/”$valid “\x1b8/dev/”$invalid “\x1b8/dev/”$valid “\x1b8/dev/”$invalid “\x1b5” )
sleep 2
for i in “${commands[@]}” do echo -ne $i | nc -u -q 0 127.0.0.1 6789 sleep 1 done
==13591== HEAP SUMMARY: ==13591== in use at exit: 8 bytes in 1 blocks ==13591== total heap usage: 23 allocs, 22 frees, 147,158 bytes allocated ==13591== ==13591== 8 bytes in 1 blocks are still reachable in loss record 1 of 1 ==13591== at 0x48455EF: calloc (vg_replace_malloc.c:1328) ==13591== by 0x10DDC7: ttys_init (ttys.c:137) ==13591== by 0x10C382: cwdaemon_cwdevice_set (cwdaemon.c:2549) ==13591== by 0x10C7DC: cwdaemon_params_cwdevice (cwdaemon.c:1768) ==13591== by 0x10C7DC: cwdaemon_handle_escaped_request (cwdaemon.c:1070) ==13591== by 0x10CF77: cwdaemon_receive (cwdaemon.c:946) ==13591== by 0x10AD94: main (cwdaemon.c:2374)
We have a simple fuzzing test in tests/fuzzing/simple/. The test needs further work:
- Decrease sleep times in test functions.
Currently the sleep time is 1 or 2 seconds, which makes the test execution longer.
This will shorten the time needed to complete the test.
- Use actual receiver in tests of requests that trigger keying of Morse code
on cwdevice.
This will demonstrate that even a fuzzed cwdaemon can key a proper message on cwdevice.
- Observe CPU usage of fuzzed cwdaemon.
This will demonstrate that fuzzed cwdaemon doesn’t fall into some unexpected state.
Come up with non-simple, non-naive fuzzing test framework.
Maybe American Fuzzy Loop? Maybe https://llvm.org/docs/LibFuzzer.html?
Requests (and to some degree replies) should not be treated as plain C strings.
It’s possible that the requests incoming from client (or from attacker) will consist of non-printable characters, and will include embedded NUL or escaped characters.
cwdaemon MUST handle such requests correctly (for legitimate users and legitimate use cases) and safely (for both legitimate and rogue users).
This means that we need to make at least following changes in cwdaemon:
- Request buffer must be changed from simple array of characters to struct with ‘bytes’ and with ‘n_bytes’ members. Similar structure already appears in tests code.
- The same change should be done for reply buffer since the buffer may also need to store non-printable characters.
- Interaction with contents of requests and replies MUST NOT be done using printf() like functions (in particular you MUST NOT call strlen() or snprintf() on request or reply buffers).
The following events must be evaluated and expectations for them must be met:
- Morse message keyed on cwdevice is correct (for plain request, caret request and REPLY Escape request).
- Reply received over socket from server is correct (for caret request and REPLY Escape request).
Some functional tests require presence of cwdevice. Currently the presence of the device is not checked explicitly, the test start running, and fails only after some time. This introduces unnecessary delay.
The tests could/should explicitly search for cwdevice, and if it’s not present then fail quickly and without unnecessary delay.
The main gain from this would be to shorten execution time of tests and to have quicker feedback on the tests.
Add a functional test that confirms that cwdaemon handles gracefully a situation where cwdevice is missing.
Add to cwdaemon’s man page description of following cases:
- handling of terminating NUL in requests by cwdaemon,
- replies are terminated with ‘\r’ + ‘\n’ ONLY.
- maximal count of bytes in replies sent by cwdaemon
Currently Morse receiver can’t correctly receive the first character keyed on cwdevice. To work around this, test code has an exception allowing the first character in received Morse text to be mis-received.
After unixcw project fixes the receiver, cwdaemon’s tests should start using that receiver and should fix the code verifying received Morse text. The code doing the verification should no longer make the exception for the first character.
See morse_receive_text_is_correct() for more info.
There is a file called BUGS that is located in my local dir - it is not a part of repo. Review its contents and include the contents in the repo - perhaps in this file, as new bug items.
Add basic support for specifying ptt pin functions (KEY/PTT) to cwdaemon.init file.
You don’t have to specify any pin assignment in the file, but add enough code to the file to make it easy and fast to change default assignments.
DONE:
cwdaemon.init: test -n “$OPT_KEY” && OPTS=”$OPTS -o key=$OPT_KEY” test -n “$OPT_PTT” && OPTS=”$OPTS -o ptt=$OPT_PTT”
cwdaemon.default:
#OPT_KEY=DTR #OPT_PTT=RTS
Fuzzing test can take a long time to run. If I want to interrupt it, I want to do it in a “nice” way that won’t leave neither the test nor tested server in bad state.
Especially the state of the server is important: it must exit in clean manner to be able to release all resources properly.
Write a good handler of Ctrl-C in the fuzzing test.
When sound system is changed through <ESC>f Escape request, the cwdaemon stops toggling keying pin on cwdevice.
Problem reported by a user.
Steps to replicate.
- Start cwdaemon ./src/cwdaemon –nofork –system p –cwdevice /dev/ttyUSB0 -iii -p 7777
- Start monitoring pins of cwdevice using your preferred method. Pay attention to keying pin.
- Start nc through which you will be sending requests to server.
nc -u 127.0.0.1 7777
- In nc send any string to be played and keyed by cwdaemon
- Observe that:
- Morse code is played on sound card,
- Morse code is keyed on keying pin of cwdevice,
- cwdaemon logs info about toggling of keying pin:
[INFO ] cwdaemon: keying event “1” [INFO ] cwdaemon: keying event “0” [INFO ] cwdaemon: keying event “1” [INFO ] cwdaemon: keying event “0”
- In nc send an Escape request to change sound system to Soundcard:
<press and release Escape key> <press and release ‘f’ key> <press and release ‘s’ key>
- In nc send any string to be played and keyed by cwdaemon.
- Observe that:
- Morse code is played on sound card,
- Morse code is NOT keyed on keying pin of cwdevice, <---- This is an error
- cwdaemon DOES NOT log info about toggling of keying pin. <---- This is an indication of error
Also observe that sending RESET Escape request seems to restore proper toggling of keying pin.
cwdaemon.c specifies that priority can be in range -20..20, inclusive.
According to Linux’s man page of setpriority(), the range is -20..19.
acerion@cwdaemon 27 $ src/cwdaemon -n –tone 200 -x o cwdaemon: Not forking… debug output: stdout Press ^C to quit <---- The long waiting for OSS happens here. cwdaemon:EE: failed to create generator with sound system “OSS” cwdaemon: cwdaemon_open_libcw_output(): 836
When a second instance of cwdaemon is started and cdwdaemon can’t bind to the default port (because it’s already used by the first instance), cwdaemon should print more info indicating a possible cause of the problem.
In theory the request should accept only “0” or “1”. In practice it accepts any numerical value.
Value passed to cwdaemon through “–libcwflags=” command line option should support decimal values in HEX format (values with “0x” prefix).
This should be supported: “–libcwflags=0xabcff04”.
In order to know which version of tests code is being run (e.g. by users that report problems), I need to have an easy way to detect version of code in logs of tests.
So the logs must show git hash of commit compiled and used for tests
Currently cwdaemon doesn’t allow specifying a sound device to be used. A default device of libcw is being used instead.
For OSS, the device is /dev/audio, but on my FreeBSD 14.1 there is no such device.
One of two things (or both) need to happen:
- libcw gets better at detecting a default device to be used for playback,
- cwdaemon allows selection of sound device through command line option.
There is some info on the internet saying that fuzzy tests should be executed on binary that has sanitizers enabled.
Double-check this information and perhaps improve a procedure and code for the testing of cwdaemon with fuzzing.
Right now a request for ‘null’ cwdevice makes cwdaemon to look for such a cwdevice in /dev:
[II] Test: starting test case 0 in iteration 7 / 16: [Requesting ‘null’ cwdevice], cwdevice name = [null] [DD] cwdevice observer: ptt sink: ptt is on [II] cwdaemon client: sent 7 bytes [II] cwdaemon: ------------------- [II] cwdaemon client: sent 2 bytes [II] cwdaemon: received Escape request: “<ESC>8” / “<ESC>0x38” [II] cwdaemon client: sent 5 bytes [EE] cwdaemon: ioctl(TIOCMGET) failed for tty device [/dev/null]: Inappropriate ioctl for device [EE] cwdaemon: ioctl(PPISSTATUS) failed for lp device [/dev/null]: Inappropriate ioctl for device [II] cwdaemon: keying device used: “null” [II] cwdaemon: Requested cwdevice [null]
cwdaemon should recognize that ‘null’ is not a device in dev but a special dummy device. There should be no probing for lp or tty device called ‘null’ in dev.
Found during tests of 0.13.0 on FreeBSD.
Valgrind reports more allocs (15) than frees (10) in HEAP SUMMARY, see log below.
While this is not reflected in LEAK SUMMARY, this is still bad and needs to be addressed soon.
This particular problem was found during execution of tests/functional_tests/unattended/option_cwdevice_tty_lines/test_program with “supervisor” set to “valgrind”.
==72103== ==72103== FILE DESCRIPTORS: 3 open (3 std) at exit. ==72103== ==72103== HEAP SUMMARY: ==72103== in use at exit: 7,552 bytes in 5 blocks ==72103== total heap usage: 15 allocs, 10 frees, 153,202 bytes allocated <---- here ==72103== ==72103== LEAK SUMMARY: ==72103== definitely lost: 0 bytes in 0 blocks ==72103== indirectly lost: 0 bytes in 0 blocks ==72103== possibly lost: 0 bytes in 0 blocks ==72103== still reachable: 0 bytes in 0 blocks ==72103== suppressed: 7,552 bytes in 5 blocks ==72103== ==72103== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) –72103– –72103– used_suppression: 1 MEMCHECK-LIBC-REACHABLE-1 /usr/local/libexec/valgrind/default.supp:595 suppressed: 4,096 bytes in 1 blocks –72103– used_suppression: 4 MEMCHECK-LIBTHR-REACHABLE-1 /usr/local/libexec/valgrind/default.supp:665 suppressed: 3,456 bytes in 4 blocks ==72103== ==72103== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Analysis with valgrind’s “massif” tool (https://stackoverflow.com/questions/30512000/how-to-make-valgrind-log-all-allocations) suggests that the problem is either in libc, or in libcw.
This problem occurs on FreeBSD, but not on Linux. On Linux the Count of frees matches the count of allocs.
closelog() should be called to close an (implicitly) opened syslog output.
The following issue has been found on FreeBSD 14.1-RELEASE during execution of tests/functional_tests/unattended/option_port/test_program test under valgrind (search for “here” word to see the exact place):
acerion@FreeBSD:~/cwdaemon/cwdaemon_git_head $ tests/functional_tests/unattended/option_port/test_program current:revision:age: 8:0:0 [II] Test: random seed: 0x87506e7b (2270195323)
[II] Test: starting test case 1 / 8: [failure case: port 0] [WW] Test: requested value of port is out of range: 0, continuing with the value anyway /usr/local/bin/valgrind -s –leak-check=full –show-leak-kinds=all –track-fds=yes /home/acerion/cwdaemon/cwdaemon_git_head/src/cwdaemon –port 0 -T 792 -x n -n -d cuaU0 -s 19 ==65473== Memcheck, a memory error detector ==65473== Copyright (C) 2002-2024, and GNU GPL’d, by Julian Seward et al. ==65473== Using Valgrind-3.23.0 and LibVEX; rerun with -h for copyright info ==65473== Command: /home/acerion/cwdaemon/cwdaemon_git_head/src/cwdaemon –port 0 -T 792 -x n -n -d cuaU0 -s 19 ==65473== ==65473== ==65473== FILE DESCRIPTORS: 4 open (3 std) at exit. ==65473== Open AF_UNIX socket 3: <unknown> ==65473== at 0x52B898A: socket (in /lib/libc.so.7) <---- here ==65473== by 0x5237C5A: ??? (in /lib/libc.so.7) ==65473== by 0x52377B0: ??? (in /lib/libc.so.7) ==65473== by 0x5237205: syslog (in /lib/libc.so.7) ==65473== by 0x404898: log_message (log.c:140) ==65473== by 0x4059AE: cwdaemon_option_network_port (options.c:51) ==65473== by 0x402173: cwdaemon_args_process_short (cwdaemon.c:1664) ==65473== by 0x402173: cwdaemon_args_process_long (cwdaemon.c:1634) ==65473== by 0x402173: cwdaemon_args_parse (cwdaemon.c:2120) ==65473== by 0x402173: main (cwdaemon.c:2167) ==65473==
This problem occurs when invalid value of “port” command-line option has been passed, and cwdaemon is logging an error to the only message output available at this time: to syslog. syslog output is probably implicitly opened by call to syslog(), before an explicit call to openlog().
A better control over opening and closing of syslog() must be exercised.
Delays requested through PTT_DELAY Escape request’s values with integer value larger than 50 are clipped by cwdaemon.
Current tests don’t cover this case carefully enough, but they should. TODO: improve the tests.
The request is not really described in the man page.
There is a short mention about it, but the man page doesn’t explain what the request really does.
Each run of cwdaemon allocates some memory on heap, even when Null sound system is being used. There is no memory leak, but the amount of allocated memory seems high.
From valgrind report:
==6437== ==6437== FILE DESCRIPTORS: 3 open (3 std) at exit. ==6437== ==6437== HEAP SUMMARY: ==6437== in use at exit: 0 bytes in 0 blocks ==6437== total heap usage: 9 allocs, 9 frees, 147,012 bytes allocated ==6437== ==6437== All heap blocks were freed – no leaks are possible ==6437== ==6437== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
147 kB of memory? I did not expect that much!
TODO: find out where this memory is allocated, and whether we can decrease the total size of allocations.
Steps to reproduce:
- ./configure –enable-functional-tests –enable-long-functional-tests
- make && make check
- Plug in USB-to-UART converter
- Make tests use Null sound system and run under valgrind:
export CWDAEMON_TEST_SOUND_SYSTEM=null export CWDAEMON_TEST_SUPERVISOR=valgrind
- Run a test:
tests/functional_tests/unattended/request_esc_exit/test_program
- Observe valgrind’s summary at the end of the test.
Right now (0.13.0) it doesn’t work.
The target works on Linux, so the priority of this ticket is only “#C”.
Detect an existing instance of cwdaemon.
Such instance may be a sign of problems during earlier execution of some test.
Refuse to conduct a test in such situation, let tester investigate why there is already a running instance of cwdaemon - was there some error or is it expected (e.g. there is a system-wide cwdaemon started by init system).
Values (“dtr”“ptt”“none”) passed throug “-options” command line options for tty cwdevice should be treated in case-insensitive way.
The test code in tests/functional_tests/unattended/option_cwdevice_tty_lines/ only uses lower-case values.
TODO: make the test use lower- and upper-case values.
NEWS file for 0.13.0 includes the following notice:
“
- A fix to GitHub issue #12 (mentioned in previous bullet point)
introduces a change/fix to resetting of tty cwdevice’s pin functions.
Until now (since v0.11.0) when cwdaemon received “change cwdevice” Escape request, the assignment of functions (keying/ptt) to tty pins (DTR, RTS) specified through command line options would be lost during handling of the Escape request. Handling of the request would reset the assignment to cwdaemon’s defaults, which may be different than assignment specified through command line options.
A commit that fixes GitHub issue #12 also fixes the problem described above. Now any changes of cwdevice through “change cwdevice” Escape request will preserve assignment of functions to tty lines specified through command line options (if any assignment was made).
“
TODO: write a test for it.