windows: take the adapter names netsh accepts, and find a built agent - #135
Merged
Conversation
OpenIPC/firmware#2381 recovered a bricked Hi3516CV300 as far as "Phase 2: Flash via TFTP" and stopped there, on four separate faults. `list_interfaces()` took its names from `socket.if_nameindex()`, which answers on Windows with "ethernet_0". netsh addresses adapters by their friendly name, has never heard of that, and says so as "Failed to configure the DHCP service. The interface may be disconnected." -- which sends the reader after a cable. The reporter got past it by renaming their adapter to `ethernet_0`. Ask netsh for the list instead, parsed by column position because netsh is localised, so the names we hand out are names we can hand back. `add_ip` treated an address that was already up as fatal, so setting it up by hand first did not help either. Bindability now decides, before and after the command: it is the property the caller actually wants, and unlike command output it is not translated. An address we did not add is no longer removed on the way out -- taking away the operator's own static IP is not ours to do. netsh also returns before the stack can bind the address, so the TFTP bind raced it and lost with EADDRNOTAVAIL. The failure unwound the context manager, which removed the address, so the log showed defib taking away what it had just assigned. `add_ip` now waits for the address to come up. Separately, `get_agent_binary` searched exactly one path: the git checkout four levels above the module. In an installed package that resolves inside site-packages and can never exist, so the whole `defib agent` family was unreachable for anyone who installed defib the documented way -- and the refusal, "No agent binary for 'hi3516cv300'", reads as an unsupported chip on a chip that is supported. It now searches DEFIB_AGENT_DIR, a packaged binaries/ directory, the cache directory and the checkout, tries both the chip name and the mapped build name so a `make SOC=gk7205v300` output is found, and when it comes up empty says what to build and how. hi3516ev200 has had its own stanza in agent/Makefile all along but was missing from the chip map, so `defib agent` refused a chip the agent builds. A test now reads the SOC list out of the Makefile and fails if the map falls behind it again.
PR Summary by QodoFix Windows temporary IP setup and installed agent discovery
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Ruff caught the `sys` import the mocked tests did not need, since they patch `ip_manager.sys` rather than their own. Putting it back for a skipif: the CI matrix runs on windows-latest, so the one claim the interface fix rests on -- that netsh can be asked for names netsh will accept -- can be checked against the real command there instead of only against a captured table.
Three findings from the review of this branch, all of them mine. `_windows_interfaces()` ran `subprocess.run` with a ten-second timeout, and `list_interfaces()` is reached from the async install and network paths and from add_ip's own failure advice. Blocking the loop there stops every other task, including the serial link to a camera sitting in its bootrom window, which does not wait for us. The netsh table parser is now separate from the call that produces it, so `list_interfaces_async()` can reuse it over `asyncio.create_subprocess_exec` while the synchronous `list-interfaces` command keeps the synchronous one. `add_ip` raised when the bindability wait timed out, having already assigned the address -- and `temporary_ip` cannot clean up, because it does not learn that we own the address until add_ip returns. So a delayed address left the host reconfigured, and the next run read it as the operator's and left it alone for good. Every exit after a successful assignment now takes the address back down, cancellation included, and a rollback that itself fails does not replace the error that caused it. `_agent_search_path` called `get_agent_cache_dir()` eagerly, and that went through `get_cache_dir()`, which creates the directory. An unwritable cache location therefore raised out of `get_agent_binary` even when DEFIB_AGENT_DIR held the binary, and out of the help text whose whole job is to say that nothing was found. Building a search path is now free of side effects; `get_cache_dir` gained `create=False` for callers that are only looking.
This was referenced Sep 8, 2026
openipc-ai
added a commit
that referenced
this pull request
Sep 8, 2026
test_real_netsh_yields_at_least_one_adapter failed all three windows-latest jobs here, on code identical to what it passed on in #135. It asserted that netsh lists at least one adapter, which is a property of the runner rather than of anything we wrote -- so it passed once, failed the next time, and told us nothing about the parser either way. What it now asserts is ours: netsh is callable, its output parses, and nothing that is not an adapter name comes back. A header row surviving the shape test is the failure mode that would matter, and that shows up regardless of what the host has plugged in. The deterministic tests against a captured table still pin the parsing itself. The failure did expose something real. The synchronous path ignored netsh's exit status, unlike the async twin added in the same PR, so a netsh that failed to run left us parsing empty stdout and reporting "no adapters" -- a command that did not run, presented as a host without a network. It checks the exit status now, and a test pins that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four faults, all found by one reporter recovering a bricked Hi3516CV300 on Windows in OpenIPC/firmware#2381. They got
defib burnworking, reachedPhase 2: Flash via TFTP, and stopped there.1. The adapter name was one netsh cannot accept
list_interfaces()took its names fromsocket.if_nameindex(), which answers on Windows withethernet_0.netsh interface ip add addressaddresses adapters by their friendly name and has never heard of that, so it failed with:which sends the reader after a cable. The reporter got past it by renaming their Windows adapter to
ethernet_0to match.Names now come from
netsh interface show interface, parsed by column position rather than header text because netsh is localised. Names with spaces (Ethernet 2is a stock Windows name) survive.2. An address that was already up was fatal
netshanswers "The object already exists",add_ipraised, so setting the IP up by hand beforehand did not help either. Bindability now decides, before and after the command — it is the property the caller actually wants (the TFTP server binds this address), and unlike command output it is not translated.add_ipreturns whether it assigned the address, andtemporary_ipremoves it only in that case. Taking away the operator's own static IP on the way out is not ours to do.3. The bind raced the address coming up
netsh returns before the stack can bind the address, so the TFTP bind that follows immediately failed with
EADDRNOTAVAIL. That unwound theAsyncExitStack, which removed the address — so the log read as though defib had taken away what it had just assigned:The reporter's reading — "Why is it REMOVING the ip it just added before trying to bind TFTPd to it?" — was right about the sequence and the cleanup is the symptom.
add_ipnow polls until the address is bindable before returning.4.
defib agentcould never work from an installed defibget_agent_binarysearched exactly one path: the git checkout four levels above the module. In an installed package that resolves insidesite-packagesand can never exist, so the wholedefib agentfamily was unreachable for anyone who installed defib the documented way. The refusal —— also reads as an unsupported chip, on a chip that is supported. It now searches
DEFIB_AGENT_DIR, a packagedbinaries/directory, the cache directory and the checkout; tries both the chip name and the mapped build name, so amake SOC=gk7205v300output is found even though that chip shares the gk7205v200 map; and when it comes up empty says what to build and how.While there:
hi3516ev200has had its own stanza inagent/Makefileall along but was missing from the chip map, sodefib agentrefused a chip the agent builds. A test now reads the SOC list out of the Makefile and fails if the map falls behind it again.Testing
Two new test files, 27 tests, plus the full suite:
I have no Windows machine of my own, but the CI matrix runs
windows-latest— so the one claim the interface fix rests on, that netsh can be asked for names netsh will accept, is checked against the real command there. The rest of the Windows paths are driven by tests that feed the netsh table and the command results directly. The end-to-end flash on real hardware is still unverified; the reporter is mid-recovery and willing to retry.