amboar
cmd: migrate from getopt to argp
Reopened because of fail in #71 / #70
The argument handling is quite hard to make it good when using getopt with help and usage messages. The argp library is a better choice for this task.
Note: cleanup of dirty code or unwanted redundancies will be done from time to time in this PR. If you notice anything that bothers you, please add a comment directly. Thanks!
Checklist:
- [x] Migrate command base structure to argp
- [ ] Refactor
host_initandast_ahb_accessfor standardised flags - [ ] Cleanup inconsistencies
- [ ] Consolidate possible duplicates and make it more user-friendly
- [ ] Add proper git commit messages
- [ ] Documentation
@amboar I don't have an AST2600 system on-hand to test the coprocessor stuff. Could you please test if the command still works as before?
@amboar I don't have an AST2600 system on-hand to test the coprocessor stuff. Could you please test if the command still works as before?
Looks like the argument processing is a bit off:
root@ast2600:~# ./culvert -vv coprocessor run 0xba000000 $((32 * 1024 * 1024)) < sclydesdale.bin
[*] Found 5 registered bridge drivers
[*] Trying bridge driver p2a
[*] Trying bridge driver ilpc
[*] Trying bridge driver devmem
[*] Trying bridge driver debug-uart
[*] Opening 33554432
[*] Failed to initialise the console (33554432): -1
[*] Failed to initialise local debug interace on 33554432: -1
[*] Trying bridge driver l2a
[*] Bridge discovery failed, cannot access BMC AHB
[*] Failed to acquire AHB interface
It should instead look like:
root@ast2600:~# ./culvert -vv coprocessor run 0xba000000 $((32 * 1024 * 1024)) < sclydesdale.bin
[*] Found 5 registered bridge drivers
[*] Trying bridge driver p2a
[*] Failed to initialise P2A bridge: -2
[*] Trying bridge driver ilpc
[*] Failed to initialise iLPC bridge: -95
[*] Trying bridge driver devmem
[*] Probing devmem
[*] Probing for SoC revision registers
[*] ahb_readl: 0x1e6e2004: 0x05030303
[*] ahb_readl: 0x1e6e207c: 0x00000000
[*] ahb_readl: 0x1e6e2014: 0x05030303
[*] Found revision 0x5030303
[*] Trying bridge driver debug-uart
[*] Unrecognised argument list for debug interface (0)
...
Looks like the argument processing is a bit off:
Found the issue. That was a leftover when I made the subcommand stuff easier š
New commit pushed where the argument processing for host_init is now correct.
Currently:
root@ast2600:~# ./culvert -vv coprocessor run 0xba000000 $((32 * 1024 * 1024)) < sclydesdale.bin
[*] Found 5 registered bridge drivers
[*] Trying bridge driver p2a
[*] Trying bridge driver ilpc
[*] Trying bridge driver devmem
[*] Trying bridge driver debug-uart
[*] Unrecognised argument list for debug interface (2)
[*] Trying bridge driver l2a
[*] Bridge discovery failed, cannot access BMC AHB
[*] Failed to acquire AHB interface
:slightly_smiling_face:
I suspect there's a stray two arguments being passed to devmem_driver_probe(), which then takes an early-exit.
I suspect there's a stray two arguments being passed to devmem_driver_probe(), which then takes an early-exit.
I'll migrate all commands and then implement a generic struct for the access stuff that's being used in like every command.
That should make it easier I'd say instead of fiddling with argc and argv shit again like it has been done before.
All commands now use argp, but they are still somewhat inconsistent, with parsing handled differently in some cases.
I will work on resolving these inconsistencies and aim to make the implementation more generic.
For readers and contributors, Iād appreciate your feedback on the documentation for the commands! Some commands are based on guesses regarding their functionality, so it would be helpful to ensure the documentation is accurate.
@sinuscosinustan something I have been idly considering for a while is to require a keyword like via (in the vein of ip route ...) to help sync the argument parsing where the user wants a specific interface to be used, e.g:
# culvert write firmware via /dev/ttyUSB0
or maybe even with the bridge name:
# culvert write firmware via debug /dev/ttyUSB0
What do you think?
This might help where there are other positional arguments, such as in the case of the coprocessor run subcommand (which I've just tested again and isn't yet working).
For the record, I plan to tag a release of culvert immediately prior to merging your changes. I think it's reasonable that we can break the command line behaviour a bit subsequent to the tag, in the process of cleaning up the implementation.
Thanks again for your effort here. I'm honestly not sure how you've motivated yourself through it, because I was certainly struggling to motivate myself :sweat_smile:
What do you think?
I was thinking about a generic set of options like -I /dev/ttyUSB0 -H <IP> and so on, but using via is also a cool idea!
For the record, I plan to tag a release of culvert immediately prior to merging your changes
Awesome! At least some breakage possible š
I'm honestly not sure how you've motivated yourself through it
Holiday, a lot of caffeine and suffering from the broken parsing š
I've pushed commit ae1c35c as proposal (I know it'll cause pointer errors and whatsoever in its current state).
@amboar What do you think about this? Would this be suitable or would be the via approach better?
I've pushed commit ae1c35c as proposal (I know it'll cause pointer errors and whatsoever in its current state).
@amboar What do you think about this? Would this be suitable or would be the
viaapproach better?
Sorry for the delay; after a quick look I think I prefer the via approach? I'm happy to prototype it myself if you don't prefer that direction (you've done a lot of great work already and I don't want you to feel obligated to any requests on my part).
after a quick look I think I prefer the
viaapproach?
Sounds good! I've tried to take a look into implementing my approach but for some reason argp doesn't want me to use the children option for that and always segfaults with non-explainable coredumps (lol).
I'm happy to prototype it myself if you don't prefer that direction
If you have a small prototype of how it should look like I'd be happy to implement it! In the meanwhile I'll rework the host_init stuff to use struct connection instead and cleanup any inconsistencies.
@amboar I have pushed a new proposal with the via keyword. Yes, it may not look nice rn (host_init_new is a temporary function which will be removed when the new approach is better) but the way how iproute2 is doing it is waaaay more disgusting.
@sinuscosinustan so you've motivated me to do something I've been meaning to get to for a while: convert the commands to use autodata.
Can you please take a look at https://github.com/amboar/culvert/pull/77 ?
Regarding your argp rework, the new struct cmd gives us a place to capture argp children. We can hook them into the reference instance, which I think is interesting?
Can you please take a look at https://github.com/amboar/culvert/pull/77 ?
I like the autodata idea. That's definitely an improvement and will make it easier for implementation, although subcommand handling might still be a bit funky (but easier).
the new
struct cmdgives us a place to capture argp children
I have looked into argp children in a playground project, but for some reason the input struct from the state is not being passed into the children's parser (and the children approach does not allow things like via at the end easily). IMO, it'll be easier to not use argp childrens.
We can hook them into the reference instance, which I think is interesting?
We can definitely use the reference instance and combine the subcommand stuff with autodata which will reduce the overall code complexity.
@sinuscosinustan are you happy to try rework the argp bits on top of the autodata changes? If not I'll have a go at rebasing your series.
are you happy to try rework the argp bits on top of the autodata changes?
I have started to rebase it last week and will do some further changes for an easier implementation of subcommands.
Half a year later and I finally found some time and motivation again šø
I rebased it and modified my initial implementation "a bit". Not all commands are ready yet (read, replace, trace and write are missing), but the majority are.
The initial implementation had a few flaws and became a bit hard to maintain due to the cascading argp parsers. Now, there's an independent root argp parser in culvert.c and each cmd only has one argp parser which acts on its own. We do not have to mess with shifting arg_num or whatsoever anymore ^^
Also, the connection_args struct is now being used in all bridge implementations and allows the generic parsing with the via argument (as you've suggested). Reworking the variadic functions for host_init will be out-of-scope here.
There are still a few inconsistencies that I want to address (mainly because I found better ways to solve an issue after reworking a command). If you spot inconsistencies, bugs or have suggestions, please feel free to leave a comment already ^^
I did a build with ASAN enabled and oops š
Building from main has the same so that's nothing new.
`culvert probe` on AST2500
# ./culvert-debug --verbose probe
[*] Found 5 registered bridge drivers
[*] Trying bridge driver l2a
[*] Failed to initialise L2A bridge: -95
[*] Trying bridge driver ilpc
[*] Probing ilpc
[*] Probing 0x2e for SuperIO
[*] Unlocking SuperIO: 0
[*] Selecting SuperIO device 2 (SUART1): 0
[*] Found device 2 selected: 0
[*] Selecting SuperIO device 12 (SUART4): 0
[*] Found device 12 selected: 0
[*] Locking SuperIO
[*] Found SuperIO device at 0x2e
[*] Probing for SoC revision registers
[*] ahb_readl: 0x1e6e2004: 0xf70ea098
[*] ahb_readl: 0x1e6e207c: 0x04030303
[*] Found revision 0x4030303
[*] Trying bridge driver devmem
[*] failed to initialise devmem bridge: -1
[*] Trying bridge driver debug-uart
[*] No interface for debug provided, skipping...
[*] Trying bridge driver p2a
[*] Probing p2a
[*] Probing for SoC revision registers
[*] ahb_readl: 0x1e6e2004: 0xffffffff
[*] ahb_readl: 0x1e6e207c: 0xffffffff
[*] Found revision 0xffffffff
[*] Revision 0xffffffff is unsupported
[*] Failed P2A probe: -19
[*] Accessing the BMC's AHB via the ilpc bridge
[*] Probing for SoC revision registers
[*] ahb_readl: 0x1e6e2004: 0xf70ea098
[*] ahb_readl: 0x1e6e207c: 0x04030303
[*] Found revision 0x4030303
[*] Selected devicetree for SoC 'aspeed,ast2500'
[*] Found 16 registered drivers
[*] Processing devicetree node at /aliases
[*] Processing devicetree node at /memory@80000000
[*] Processing devicetree node at /ahb
[*] Processing devicetree node at /ahb/sram@1e720000
[*] Processing devicetree node at /ahb/bus-controller@1e600000
[*] Bound trace driver to /ahb/bus-controller@1e600000
[*] Processing devicetree node at /ahb/apb
[*] Processing devicetree node at /ahb/apb/spi@1e620000
[*] Bound sfc driver to /ahb/apb/spi@1e620000
[*] Processing devicetree node at /ahb/apb/spi@1e630000
[*] Bound sfc driver to /ahb/apb/spi@1e630000
[*] Processing devicetree node at /ahb/apb/spi@1e631000
[*] Bound sfc driver to /ahb/apb/spi@1e631000
[*] Processing devicetree node at /ahb/apb/memory-controller@1e6e0000
[*] Bound sdmc driver to /ahb/apb/memory-controller@1e6e0000
[*] Processing devicetree node at /ahb/apb/syscon@1e6e2000
[*] Processing devicetree node at /ahb/apb/syscon@1e6e2000/clock
[*] Bound clk driver to /ahb/apb/syscon@1e6e2000/clock
[*] Processing devicetree node at /ahb/apb/syscon@1e6e2000/strapping
[*] Bound strap driver to /ahb/apb/syscon@1e6e2000/strapping
[*] Processing devicetree node at /ahb/apb/syscon@1e6e2000/superio
[*] Bound sioctl driver to /ahb/apb/syscon@1e6e2000/superio
[*] Processing devicetree node at /ahb/apb/syscon@1e6e2000/bridge-controller
[*] Bound bridge-controller driver to /ahb/apb/syscon@1e6e2000/bridge-controller
[*] Processing devicetree node at /ahb/apb/syscon@1e6e2000/debug-bridge-controller
[*] Bound debugctl driver to /ahb/apb/syscon@1e6e2000/debug-bridge-controller
[*] Processing devicetree node at /ahb/apb/syscon@1e6e2000/pcie-bridge-controller
[*] Bound pciectl driver to /ahb/apb/syscon@1e6e2000/pcie-bridge-controller
[*] Bound scu driver to /ahb/apb/syscon@1e6e2000
[*] Processing devicetree node at /ahb/apb/jtag@1e6e4000
[*] Bound jtag driver to /ahb/apb/jtag@1e6e4000
[*] Processing devicetree node at /ahb/apb/watchdog@1e785000
[*] Bound wdt driver to /ahb/apb/watchdog@1e785000
[*] Processing devicetree node at /ahb/apb/watchdog@1e785020
[*] Bound wdt driver to /ahb/apb/watchdog@1e785020
[*] Processing devicetree node at /ahb/apb/watchdog@1e785040
[*] Bound wdt driver to /ahb/apb/watchdog@1e785040
[*] Processing devicetree node at /ahb/apb/serial@1e787000
[*] Bound vuart driver to /ahb/apb/serial@1e787000
[*] Processing devicetree node at /ahb/apb/lpc@1e789000
[*] Processing devicetree node at /ahb/apb/lpc@1e789000/bridge-controller
[*] Bound ilpcctl driver to /ahb/apb/lpc@1e789000/bridge-controller
[*] Bound uart-mux driver to /ahb/apb/lpc@1e789000
[*] ahb_readl: 0x1e6e2000: 0x00000000
[*] Unlocking SCU
[*] ahb_writel: 0x1e6e2000: 0x1688a8a8
[*] Initialised scu driver
[*] Initialised strap driver
[*] Initialised sioctl driver
[*] Initialised ilpcctl driver
[*] Initialised ilpcctl AHB bridge controller
[*] fdt: Searching devicetree for type 'memory'
[*] Initialised sdmc driver
[*] Initialised pciectl driver
[*] Initialised pciectl AHB bridge controller
[*] Initialised bridge-controller driver
[*] Initialised debugctl driver
[*] Initialised debugctl AHB bridge controller
[*] ahb_readl: 0x1e6e202c: 0x00200401
debug: Disabled
[*] Failed to find 'bridge-gate-names' node: -1
[*] Failed to resolve bridge gate for gate name '(null)': -22
[*] Failed to query bridge state for BMC XDMA: -22
[*] Failed to assess XDMA bridge status: -22
[*] Failed to read P2A bridge status: -22
[*] Failed to generate xdma report: -22
[*] Failed to find 'bridge-gate-names' node: -1
[*] Failed to resolve bridge gate for gate name '(null)': -22
[*] Failed to query bridge state for BMC MMIO: -22
[*] Failed to read P2A bridge status: -22
[*] Failed to generate p2a report: -22
[*] ahb_readl: 0x1e6e2070: 0xf100c096
[*] ahb_readl: 0x1e789100: 0x00000050
ilpc: Restricted
[*] ahb_readl: 0x1e6e2070: 0xf100c096
SuperIO address: 0x2e
[*] Failed to probe SoC bridge controllers: -22
[*] Unbound instance of driver uart-mux
[*] Unbound instance of driver ilpcctl
[*] Unbound instance of driver vuart
[*] Unbound instance of driver wdt
[*] Unbound instance of driver wdt
[*] Unbound instance of driver wdt
[*] Unbound instance of driver jtag
[*] Unbound instance of driver scu
[*] Unbound instance of driver pciectl
=================================================================
==14615==ERROR: AddressSanitizer: heap-use-after-free on address 0x5080000000c0 at pc 0x5d3e092a8de4 bp 0x7ffc62ada910 sp 0x7ffc62ada900
WRITE of size 8 at 0x5080000000c0 thread T0
#0 0x5d3e092a8de3 in list_del_ ../src/ccan/list/list.h:327
#1 0x5d3e092a8de3 in soc_bridge_controller_unregister ../src/soc.c:585
#2 0x5d3e092a8de3 in debugctl_driver_destroy ../src/soc/debugctl.c:191
#3 0x5d3e092ad615 in soc_unbind_drivers ../src/soc.c:185
#4 0x5d3e092ad615 in soc_destroy ../src/soc.c:218
#5 0x5d3e092c4076 in do_probe ../src/cmd/probe.c:129
#6 0x5d3e092a7574 in main ../src/culvert.c:183
#7 0x771a75e2a1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
#8 0x771a75e2a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
#9 0x5d3e092a7854 in _start (/root/culvert-debug+0x19854) (BuildId: a6b0876b1feb01c6310201550e166da3e71802a6)
0x5080000000c0 is located 32 bytes inside of 96-byte region [0x5080000000a0,0x508000000100)
freed by thread T0 here:
#0 0x771a762fc4d8 in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
#1 0x5d3e092ad615 in soc_unbind_drivers ../src/soc.c:185
#2 0x5d3e092ad615 in soc_destroy ../src/soc.c:218
#3 0x5d3e092c4076 in do_probe ../src/cmd/probe.c:129
#4 0x5d3e092a7574 in main ../src/culvert.c:183
#5 0x771a75e2a1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
#6 0x771a75e2a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
#7 0x5d3e092a7854 in _start (/root/culvert-debug+0x19854) (BuildId: a6b0876b1feb01c6310201550e166da3e71802a6)
previously allocated by thread T0 here:
#0 0x771a762fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x5d3e092d6720 in pciectl_driver_init ../src/soc/pciectl.c:767
#2 0x5d3e092bf214 in soc_device_init_driver.part.0.lto_priv.0 ../src/soc.c:506
#3 0x5d3e092c3c4b in soc_device_init_driver ../src/soc.c:487
#4 0x5d3e092c3c4b in soc_init_bridge_controllers ../src/soc.c:598
#5 0x5d3e092c4181 in soc_probe_bridge_controllers ../src/soc.c:623
#6 0x5d3e092c4181 in do_probe ../src/cmd/probe.c:119
#7 0x5d3e092a7574 in main ../src/culvert.c:183
#8 0x771a75e2a1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
#9 0x771a75e2a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
#10 0x5d3e092a7854 in _start (/root/culvert-debug+0x19854) (BuildId: a6b0876b1feb01c6310201550e166da3e71802a6)
SUMMARY: AddressSanitizer: heap-use-after-free ../src/ccan/list/list.h:327 in list_del_
Shadow bytes around the buggy address:
0x507ffffffe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x507ffffffe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x507fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x507fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x508000000000: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
=>0x508000000080: fa fa fa fa fd fd fd fd[fd]fd fd fd fd fd fd fd
0x508000000100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x508000000180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x508000000200: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x508000000280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x508000000300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==14615==ABORTING
`culvert probe` on AST2520 (unsupported platform)
$ sudo ./build/src/culvert --verbose probe
[*] Found 5 registered bridge drivers
[*] Trying bridge driver l2a
[*] Failed to initialise L2A bridge: -95
[*] Trying bridge driver ilpc
[*] Probing ilpc
[*] Probing 0x2e for SuperIO
[*] Unlocking SuperIO: 0
[*] Selecting SuperIO device 2 (SUART1): 0
[*] Found device 255 selected: 0
[*] Locking SuperIO
[*] Probing 0x4e for SuperIO
[*] Unlocking SuperIO: 0
[*] Selecting SuperIO device 2 (SUART1): 0
[*] Found device 2 selected: 0
[*] Selecting SuperIO device 12 (SUART4): 0
[*] Found device 12 selected: 0
[*] Locking SuperIO
[*] Found SuperIO device at 0x4e
[*] Probing for SoC revision registers
[*] ahb_readl: 0x1e6e2004: 0xf74ff8d8
[*] ahb_readl: 0x1e6e207c: 0x04030103
[*] Found revision 0x4030103
[*] Revision 0x4030103 is unsupported
[*] Failed iLPC probe: -19
[*] Trying bridge driver devmem
[*] failed to initialise devmem bridge: -1
[*] Trying bridge driver debug-uart
[*] No interface for debug provided, skipping...
[*] Trying bridge driver p2a
[*] Probing p2a
[*] Probing for SoC revision registers
[*] ahb_readl: 0x1e6e2004: 0xf74ff8d8
[*] ahb_readl: 0x1e6e207c: 0x04030103
[*] Found revision 0x4030103
[*] Revision 0x4030103 is unsupported
[*] Failed P2A probe: -19
[*] Bridge discovery failed, cannot access BMC AHB
[*] Failed to acquire AHB interface, exiting
=================================================================
==3025197==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 56 byte(s) in 1 object(s) allocated from:
#0 0x788c4c2fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x620e8540735d in debug_driver_probe ../src/bridge/debug.c:540
#2 0x620e853f4f64 in host_probe_bridge ../src/host.c:100
#3 0x620e853f4f64 in host_init ../src/host.c:136
#4 0x620e8540ce7d in do_probe ../src/cmd/probe.c:98
#5 0x620e853f0574 in main ../src/culvert.c:183
#6 0x788c4be2a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#7 0x788c4be2a28a in __libc_start_main_impl ../csu/libc-start.c:360
#8 0x620e853f0854 in _start (/home/sinuscosinustan/culvert/build/src/culvert+0x19854) (BuildId: a6b0876b1feb01c6310201550e166da3e71802a6)
SUMMARY: AddressSanitizer: 56 byte(s) leaked in 1 allocation(s).
@amboar Do you think it'd be feasible to ask people on Discord to test this branch to find out if there are bugs / regressions? So far I've been using the branch for two weeks now and haven't triggered any issues on AST2500 and 2600 systems. I couldn't test coprocessor that much since I don't have a working Zephyr binary šø
Done:
- https://discord.com/channels/775381525260664832/867820390406422538/1404613391808598206
- https://discord.com/channels/775381525260664832/922871693008068638/1404613336406036560