Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
*~
/*.o
/microcom
*.pyc

# autotools stuff
/.deps
Expand Down
4 changes: 2 additions & 2 deletions can.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
104 changes: 55 additions & 49 deletions microcom.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ static struct termios sots; /* old stdout/in termios settings to restore */

struct ios_ops *ios;
int debug;
int quiet;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#55 explicitly zeros debug. Not zeroing quiet now aligns it with the code as-is, but since #55 should be merged first I think this should be: int quiet = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's an artifact from my attempt to split up what was originally a single series. quiet is zero-initialized on the "single series" branch to which I can update this pr once the dependent PRs are merged.

I suppose unitialized globals are more a bad style practice than actual reliance on unitialized memory, at least on linux where they're zero-initialized as part of the bss (in contrast to locals which are stack-allocated and not automatically initialized).

Anyway, yes this will be taken care of :)


void init_terminal(void)
{
Expand Down Expand Up @@ -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=<logfile> log output to <logfile>\n"
" -o, --listenonly Do not modify local terminal, do not send input\n"
" from stdin\n"
Expand Down Expand Up @@ -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'},
Expand All @@ -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;
Comment on lines +152 to +200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mixes a formatting change with a code change. It would be great to only have the

+		case 'q':
+			quiet = 1;
+			break;

In this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally based this on all the three dependent branches and only haphazardly rebased to main to avoid making too large a pull request.
The diff here will be more meaningful once #55 is closed. I'm converting this pr to draft until then.

}
}

Expand Down Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion microcom.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion serial.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion telnet.c
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
24 changes: 24 additions & 0 deletions test/conftest.py
Original file line number Diff line number Diff line change
@@ -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)
101 changes: 101 additions & 0 deletions test/test_telnet.py
Original file line number Diff line number Diff line change
@@ -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):
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define helper functions directly; there is no need to use fixtures for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason that this is a fixture and not a helper function is that it directly depends on the cmd fixture. My understanding is that only tests and fixtures can depend on other fixtures, therefore a fixture is adequate here.

As there's little reason why a test that uses telnet_recv would also use cmd, there is no reason to pass the cmd fixture to the test only to then pass it on to telnet_recv.
So I'd prefer this:

def test_a(telnet_recv):
    assert telnet_recv("abc") == "abc"

Over this:

def test_a(cmd):
    assert telnet_recv(cmd, "abc") == "abc"

The difference is negligible in this case but adding more dependent fixtures to telnet_recv in the latter solution would be suboptimal, because they'd all need to be added to each of the tests even if the test code itself isn't touched

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a fixture for helper functions such as these hides that telnet_recv actually uses the cmd fixture, making the code unnecessarily hard to read.

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()
Comment on lines +28 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this hardcoded sleep result in flaky tests in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely. I'm not sure why this is even needed. I'd expect that expect blocks until microcom has connected via TCP and that from this point whatever the server sends should definitely reach microcom. I suspect this is actually a workaround for a microcom bug.
As long as it's needed, should 0.01 turn out flaky on a slow runner, 0.1 should still run the tests quick enough.

But yes bad bad hack!


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)
Comment on lines +63 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@pytest.mark.parametrize("buf", [10, 1023, 1024, 1025, 4000])
def test_no_cmd(telnet_recv, buf):
payload = make_pattern(buf)
@pytest.mark.parametrize("length", [10, 1023, 1024, 1025, 4000])
def test_no_cmd(telnet_recv, length):
payload = make_pattern(length)


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