Argument parsing for write firmware broken

[sinuscosinustan]

I encountered an issue when writing firmware via UART on a test system (as the AHB bridge is locked). The argument parsing in the debug_uart driver behaves unexpectedly when using culvert write firmware /dev/ttyUSB0 < image-bmc.

Specifically, it takes firmware as the first argument in ahb *debug_driver_probe and /dev/ttyUSB0 as the second one, causing the driver to fail since argc is 2 instead of the expected 1 or 5.

culvert/src/bridge/debug.c

Lines 518 to 535 in 8335227

	static struct ahb *debug_driver_probe(int argc, char *argv[])
	{
	struct debug *ctx;
	int rc;
	ctx = malloc(sizeof(*ctx));
	if (!ctx) {
	return NULL;
	}
	if (argc == 1) {
	/* Local debug interface */
	if ((rc = debug_init(ctx, argv[0])) < 0) {
	loge("Failed to initialise local debug interace on %s: %d\n",
	argv[0], rc);
	goto cleanup_ctx;
	}
	} else if (argc == 5) {

I hacked something in so that it is usable again, although I hate it 😄 .

Temporary hack to use it

diff --git a/src/bridge/debug.c b/src/bridge/debug.c
index 68eb41f..2510779 100644
--- a/src/bridge/debug.c
+++ b/src/bridge/debug.c
@@ -525,6 +525,13 @@ static struct ahb *debug_driver_probe(int argc, char *argv[])
         return NULL;
     }
 
+    if (argc == 2 && strcmp( argv[0], "firmware" ) == 0) {
+           logi("Hacking argc/argv to remove firmware as first argument\n");
+           argc--;
+           argv++;
+    }
+
+
     if (argc == 1) {
         /* Local debug interface */
         if ((rc = debug_init(ctx, argv[0])) < 0) {

[amboar]

I hacked something in so that it is usable again, although I hate it 😄 .

Haha, that's certainly on-brand for this tool. Almost a fundamental philosophy.

Sorry for breaking this though. I expect I broke it in cmd: {read,write}: Enable bounded access to RAM. As alluded to in #60 the argument parsing stuff is in a pretty sad state. It seems that the December break is my traditional time for hacking on culvert, so if you don't cook up a better patch in the mean-time I'll try to fix it as part of a broader argument-parsing cleanup.

[sinuscosinustan]

so if you don't cook up a better patch in the mean-time

My C skills are rather limited and I am happy that I can program my ESP chips without doing too many malloc/free's 😄

I was thinking of looking into argp (or ketopt if we want to keep the getopt syntax) since culvert can only be used on Linux anyways, but then I was like "how do you want to implement argp in a way it is usable with global arguments and maybe even independent order?" ^^ (i.e. being able to define --verbose at the end too)

[amboar]

how do you want to implement argp in a way it is usable with global arguments and maybe even independent order?" ^^ (i.e. being able to define --verbose at the end too)

Honestly I prefer we don't allow independent order. argp looks nice, I hadn't actually come across it, so thanks. I'll poke a bit more at that when I have a moment.

[sinuscosinustan]

Honestly I prefer we don't allow independent order

That makes it way easier to implement then 😄

I already have a small draft on my laptop where I migrated the console and main command to argp. If you want, I can push it and open a draft PR to check if this would suite to how it should work (although, I haven't added much docs yet). I've opened a draft PR with a first implementation (not finished yet but the implemented parts do work well).