From 53a335860fffc26ea1e9207e363861b61300a598 Mon Sep 17 00:00:00 2001 From: Jonas Rebmann Date: Fri, 12 Dec 2025 14:41:36 +0100 Subject: [PATCH 1/6] microcom: add --quiet switch The quiet switch shall silence all printing to stdout during normal operation, but leaves the interactive console and all error messages functional. Useful for automation such as in CI. Signed-off-by: Jonas Rebmann --- can.c | 4 +-- microcom.c | 104 ++++++++++++++++++++++++++++------------------------- microcom.h | 4 ++- serial.c | 2 +- telnet.c | 2 +- 5 files changed, 62 insertions(+), 54 deletions(-) diff --git a/can.c b/can.c index bf7bb61..d30eca5 100644 --- a/can.c +++ b/can.c @@ -184,8 +184,8 @@ struct ios_ops *can_init(char *interface_id) return NULL; } - printf("connected to %s (rx_id=%x, tx_id=%x)\n", - interface, filter->can_id, data.can_id); + msg_printf("connected to %s (rx_id=%x, tx_id=%x)\n", + interface, filter->can_id, data.can_id); return ios; } diff --git a/microcom.c b/microcom.c index a8810b4..b8e2fe3 100644 --- a/microcom.c +++ b/microcom.c @@ -36,6 +36,7 @@ static struct termios sots; /* old stdout/in termios settings to restore */ struct ios_ops *ios; int debug; +int quiet; void init_terminal(void) { @@ -98,6 +99,7 @@ void main_usage(int exitcode, char *str, char *dev) " default: (%s:%x:%x)\n" " -f, --force ignore existing lock file\n" " -d, --debug output debugging info\n" + " -q, --quiet do not print status information to stdout\n" " -l, --logfile= log output to \n" " -o, --listenonly Do not modify local terminal, do not send input\n" " from stdin\n" @@ -136,6 +138,7 @@ int main(int argc, char *argv[]) { "telnet", required_argument, NULL, 't'}, { "can", required_argument, NULL, 'c'}, { "debug", no_argument, NULL, 'd' }, + { "quiet", no_argument, NULL, 'q' }, { "force", no_argument, NULL, 'f' }, { "logfile", required_argument, NULL, 'l'}, { "listenonly", no_argument, NULL, 'o'}, @@ -144,54 +147,57 @@ int main(int argc, char *argv[]) { }, }; - while ((opt = getopt_long(argc, argv, "hp:s:t:c:dfl:oi:a:e:v", long_options, NULL)) != -1) { + while ((opt = getopt_long(argc, argv, "hp:s:t:c:dqfl:oi:a:e:v", long_options, NULL)) != -1) { switch (opt) { - case '?': - main_usage(1, "", ""); - break; - case 'h': - main_usage(0, "", ""); - break; - case 'v': - printf("%s\n", PACKAGE_VERSION); - exit(EXIT_SUCCESS); - break; - case 'p': - device = optarg; - break; - case 's': - current_speed = strtoul(optarg, NULL, 0); - break; - case 't': - telnet = 1; - hostport = optarg; - break; - case 'c': - can = 1; - interfaceid = optarg; - break; - case 'f': - opt_force = 1; - break; - case 'd': - debug = 1; - break; - case 'l': - logfile = optarg; - break; - case 'o': - listenonly = 1; - break; - case 'a': - answerback = optarg; - break; - case 'e': - if (strlen(optarg) != 1) { - fprintf(stderr, "Option -e requires a single character argument.\n"); - exit(EXIT_FAILURE); - } - escape_char = *optarg; - break; + case '?': + main_usage(1, "", ""); + break; + case 'h': + main_usage(0, "", ""); + break; + case 'v': + printf("%s\n", PACKAGE_VERSION); + exit(EXIT_SUCCESS); + break; + case 'p': + device = optarg; + break; + case 's': + current_speed = strtoul(optarg, NULL, 0); + break; + case 't': + telnet = 1; + hostport = optarg; + break; + case 'c': + can = 1; + interfaceid = optarg; + break; + case 'f': + opt_force = 1; + break; + case 'd': + debug = 1; + break; + case 'q': + quiet = 1; + break; + case 'l': + logfile = optarg; + break; + case 'o': + listenonly = 1; + break; + case 'a': + answerback = optarg; + break; + case 'e': + if (strlen(optarg) != 1) { + fprintf(stderr, "Option -e requires a single character argument.\n"); + exit(EXIT_FAILURE); + } + escape_char = *optarg; + break; } } @@ -239,8 +245,8 @@ int main(int argc, char *argv[]) ios->set_flow(ios, current_flow); if (!listenonly) { - printf("Escape character: Ctrl-%c\n", escape_char); - printf("Type the escape character to get to the prompt.\n"); + msg_printf("Escape character: Ctrl-%c\n", escape_char); + msg_printf("Type the escape character to get to the prompt.\n"); /* Now deal with the local terminal side */ tcgetattr(STDIN_FILENO, &sots); diff --git a/microcom.h b/microcom.h index 52c6a5b..e105586 100644 --- a/microcom.h +++ b/microcom.h @@ -72,6 +72,7 @@ void main_usage(int exitcode, char *str, char *dev); extern struct ios_ops *ios; extern int debug; +extern int quiet; extern int opt_force; extern int listenonly; extern char *answerback; @@ -121,7 +122,8 @@ extern int current_flow; int do_commandline(void); int do_script(char *script); -#define dbg_printf(fmt,args...) ({ if (debug) printf(fmt ,##args); }) +#define dbg_printf(...) do { if (debug) printf(__VA_ARGS__); } while (0) +#define msg_printf(...) do { if (!quiet) printf(__VA_ARGS__); } while (0) /* * Some telnet options according to diff --git a/serial.c b/serial.c index 87c4353..11b4a0d 100644 --- a/serial.c +++ b/serial.c @@ -267,7 +267,7 @@ struct ios_ops * serial_init(char *device) memcpy(&pots, &pts, sizeof (pots)); init_comm(&pts); tcsetattr(fd, TCSANOW, &pts); - printf("connected to %s\n", device); + msg_printf("connected to %s\n", device); return ops; } diff --git a/telnet.c b/telnet.c index 5878a01..d587a7e 100644 --- a/telnet.c +++ b/telnet.c @@ -603,7 +603,7 @@ struct ios_ops *telnet_init(char *hostport) fprintf(stderr, "getnameinfo: %s\n", gai_strerror(ret)); goto out; } - printf("connected to %s (port %s)\n", connected_host, connected_port); + msg_printf("connected to %s (port %s)\n", connected_host, connected_port); /* send intent we WILL do COM_PORT stuff */ dbg_printf("-> WILL COM_PORT_CONTROL\n"); From 00930762fb0bf3c29290d8091965b360e6e65cc3 Mon Sep 17 00:00:00 2001 From: Jonas Rebmann Date: Fri, 12 Dec 2025 14:57:12 +0100 Subject: [PATCH 2/6] test: use pytest, test telnet.c I set up those tests for the issues with the current telnet implementation. They can serve as regression tests after the issues have been resolved. Add them as the first integration tests. Add pythons bytecode cache files to gitignore. Signed-off-by: Jonas Rebmann --- .gitignore | 1 + test/conftest.py | 24 +++++++++++ test/test_telnet.py | 101 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+) create mode 100644 test/conftest.py create mode 100644 test/test_telnet.py diff --git a/.gitignore b/.gitignore index 89f2727..5b9b330 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ *~ /*.o /microcom +*.pyc # autotools stuff /.deps diff --git a/test/conftest.py b/test/conftest.py new file mode 100644 index 0000000..6531b8e --- /dev/null +++ b/test/conftest.py @@ -0,0 +1,24 @@ +import os +from pathlib import Path +import shlex +import pytest + + +def pytest_sessionstart(session): + # run tests in toplevel directory always, not in test/ + os.chdir(Path(__file__).resolve().parent.parent) + + +def pytest_addoption(parser): + parser.addoption( + "--cmd", + action="store", + default="./microcom", + help="Command used to invoke microcom" + ) + + +@pytest.fixture(scope="session") +def cmd(pytestconfig): + cmd = pytestconfig.getoption("--cmd") + return shlex.split(cmd) diff --git a/test/test_telnet.py b/test/test_telnet.py new file mode 100644 index 0000000..13b0dc7 --- /dev/null +++ b/test/test_telnet.py @@ -0,0 +1,101 @@ +import socket +import threading +import time +import pytest +import subprocess +import os +import pty + +RFC2217_CMD = bytes([255, 254, 44]) # RFC2217 IAC DONT COM-PORT-OPTION + + +def make_pattern(length): + ascii_range = list(range(32, 127)) + return bytes(ascii_range[i % len(ascii_range)] for i in range(length)) + + +@pytest.fixture +def telnet_recv(cmd): + def _recv(buf, timeout=1): + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.bind(("127.0.0.1", 0)) + sock.listen(1) + host, port = sock.getsockname() + + def run(): + conn, _ = sock.accept() + with conn: + conn.sendall(buf) + time.sleep(0.01) # sadly without this, microcom may miss the transmission + sock.close() + + thread = threading.Thread(target=run, daemon=True) + thread.start() + + telnet_cmd = cmd + [f"--telnet={host}:{port}", "--quiet"] + master_fd, slave_fd = pty.openpty() + proc = subprocess.Popen( + telnet_cmd, + stdin=slave_fd, + stdout=slave_fd, + stderr=os.dup(2), + close_fds=True, + ) + os.close(slave_fd) + output = bytearray() + start_time = time.time() + while (time.time() - start_time) < timeout: + try: + chunk = os.read(master_fd, 1024) + if not chunk: + break + output.extend(chunk) + except OSError: + break + os.close(master_fd) + proc.wait() + assert proc.returncode in (0, 1), f"Exit code must be 0 or 1, got {proc.returncode}" + + return bytes(output) + return _recv + + +@pytest.mark.parametrize("buf", [10, 1023, 1024, 1025, 4000]) +def test_no_cmd(telnet_recv, buf): + payload = make_pattern(buf) + + assert telnet_recv(payload) == payload + + +def test_cmd_across_buffers(telnet_recv): + before_pattern = make_pattern(1023) + after_pattern = make_pattern(20) + payload = before_pattern + RFC2217_CMD + after_pattern + expected_output = before_pattern + after_pattern + + assert telnet_recv(payload) == expected_output + + +def test_cmd_buffer_end(telnet_recv): + pattern = make_pattern(1023) + payload = pattern + RFC2217_CMD + + assert telnet_recv(payload) == pattern + + +def test_cmd_within_buffer(telnet_recv): + before_pattern = make_pattern(345) + after_pattern = make_pattern(890) + payload = before_pattern + RFC2217_CMD + after_pattern + expected_output = before_pattern + after_pattern + + assert telnet_recv(payload) == expected_output + + +def test_iac_escape(telnet_recv): + before_pattern = make_pattern(42) + after_pattern = make_pattern(42) + payload = before_pattern + bytes([255, 255]) + after_pattern + expected_output = before_pattern + bytes([255]) + after_pattern + + assert telnet_recv(payload) == expected_output From d23c946a084ab2b52285bb0503ce3409ad102e40 Mon Sep 17 00:00:00 2001 From: Jonas Rebmann Date: Mon, 1 Dec 2025 00:01:45 +0100 Subject: [PATCH 3/6] CI: Make build workflow usable - correct branch to main, which our default branch - fix dependency installation - invoke pytest - don't build an unneeded release tarball Signed-off-by: Jonas Rebmann --- .github/workflows/build.yml | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6951edc..6a8acff 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -2,24 +2,25 @@ name: Build test on: push: branches: - - master + - main pull_request: branches: - - master + - main jobs: build: runs-on: ubuntu-latest - steps: - uses: actions/checkout@v3 - name: Install Dependencies - run: - sudo apt install - libreadline6-dev - autoconf - automake + run: | + sudo apt update -y + sudo apt install -y \ + libreadline6-dev \ + autoconf \ + automake \ + python3-pytest - name: Prepare (autoreconf) run: autoreconf -i @@ -30,8 +31,8 @@ jobs: - name: Build run: make + - name: Run pytest + run: pytest -v + - name: Run check run: make check - - - name: Run distcheck - run: make distcheck From 3b23acc92e8399809d550734788410bd0d246eae Mon Sep 17 00:00:00 2001 From: Jonas Rebmann Date: Sun, 14 Dec 2025 20:22:11 +0100 Subject: [PATCH 4/6] CI: build and test with ASAN and UBSAN This has proven to be useful to catch out-of-bounds access and I assume will prevent many cases of success by chance when testing microcom. Signed-off-by: Jonas Rebmann --- .github/workflows/build.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6a8acff..8412a41 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -26,12 +26,18 @@ jobs: run: autoreconf -i - name: Prepare (configure) + env: + CFLAGS: "-g -O2 -Wpedantic -Werror -Wno-error=unused-result -fsanitize=address,undefined -fno-omit-frame-pointer" + LDFLAGS: "-fsanitize=address,undefined" run: ./configure - name: Build run: make - name: Run pytest + env: + ASAN_OPTIONS: "detect_leaks=1" + UBSAN_OPTIONS: "print_stacktrace=1" run: pytest -v - name: Run check From 8d31993901b6c12b0f6c59c427e7e4fef3b83b11 Mon Sep 17 00:00:00 2001 From: Jonas Rebmann Date: Fri, 12 Dec 2025 23:57:34 +0100 Subject: [PATCH 5/6] CI: enforce style using uncrustify The rules are rather flexible so they should be enforcible treewide. If not, maybe there's a way to at least only enforce it on the sections or at least files that the commits touched? Signed-off-by: Jonas Rebmann --- .github/workflows/style.yml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .github/workflows/style.yml diff --git a/.github/workflows/style.yml b/.github/workflows/style.yml new file mode 100644 index 0000000..ab4cbc5 --- /dev/null +++ b/.github/workflows/style.yml @@ -0,0 +1,21 @@ +name: Enforce coding style +on: + pull_request: + branches: + - main + +jobs: + style: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Install Dependencies + run: | + sudo apt update -y + sudo apt install -y \ + uncrustify + - name: enforce style using uncrustify + run: | + uncrustify --replace -c uncrustify.cfg *.c *.h + git diff --exit-code From 86dea610c972b872f1ddbdd70b91d46200c7c629 Mon Sep 17 00:00:00 2001 From: Jonas Rebmann Date: Sun, 14 Dec 2025 16:35:15 +0100 Subject: [PATCH 6/6] CI: build and test with valgrind memory checker too Amongst others, this catches uninitialized memory access, which libasan does not. Signed-off-by: Jonas Rebmann --- .github/workflows/build.yml | 50 +++++++++++++++++++++++++++++++------ test/test_telnet.py | 2 +- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 8412a41..0c8d75a 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -20,25 +20,61 @@ jobs: libreadline6-dev \ autoconf \ automake \ - python3-pytest + python3-pytest \ + valgrind - name: Prepare (autoreconf) run: autoreconf -i - - name: Prepare (configure) + # Build 1: ASAN + - name: Create ASAN build directory + run: mkdir -p build-asan + + - name: Configure ASAN build + working-directory: build-asan env: CFLAGS: "-g -O2 -Wpedantic -Werror -Wno-error=unused-result -fsanitize=address,undefined -fno-omit-frame-pointer" LDFLAGS: "-fsanitize=address,undefined" - run: ./configure + run: ../configure - - name: Build + - name: Build ASAN + working-directory: build-asan run: make - - name: Run pytest + - name: Run pytest with ASAN + continue-on-error: true + env: + ASAN_OPTIONS: "detect_leaks=1" + UBSAN_OPTIONS: "print_stacktrace=1" + run: pytest -v --cmd="build-asan/microcom" + + - name: Run check with ASAN + continue-on-error: true + working-directory: build-asan env: ASAN_OPTIONS: "detect_leaks=1" UBSAN_OPTIONS: "print_stacktrace=1" - run: pytest -v + run: make check + + # Build 2: Valgrind + - name: Create unoptimized build directory + run: mkdir -p build-unopt + + - name: Configure unoptimized build + working-directory: build-unopt + env: + CFLAGS: "-g -O0 -Wpedantic -Werror -Wno-error=unused-result" + run: ../configure + + - name: Build unoptimized + working-directory: build-unopt + run: make + + - name: Run pytest with Valgrind + continue-on-error: true + run: pytest -v --cmd="valgrind --tool=memcheck --error-exitcode=99 --quiet build-unopt/microcom" - - name: Run check + - name: Run check with Valgrind + continue-on-error: true + working-directory: build-unopt run: make check diff --git a/test/test_telnet.py b/test/test_telnet.py index 13b0dc7..336467b 100644 --- a/test/test_telnet.py +++ b/test/test_telnet.py @@ -54,7 +54,7 @@ def run(): break os.close(master_fd) proc.wait() - assert proc.returncode in (0, 1), f"Exit code must be 0 or 1, got {proc.returncode}" + assert proc.returncode in (0, 1) return bytes(output) return _recv