fix(renode): address review follow-ups from PR #533

python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py

@@ -22,11 +22,13 @@
 
 _ELF_MAGIC = b"\x7fELF"
 
-_ALLOWED_LOAD_COMMANDS = frozenset({
-    "sysbus LoadELF",
-    "sysbus LoadBinary",
-    "sysbus LoadSymbolsFrom",
-})
+_ALLOWED_LOAD_COMMANDS = frozenset(
+    {
+        "sysbus LoadELF",
+        "sysbus LoadBinary",
+        "sysbus LoadSymbolsFrom",
+    }
+)
 
 
 def _detect_load_command(firmware_path: str) -> str:
@@ -42,6 +44,9 @@ def _detect_load_command(firmware_path: str) -> str:
 
 
 def _find_free_port() -> int:
+    # NOTE: TOCTOU race — the port is released before Renode binds it,
+    # so another process could grab it first.  Switching to Unix domain
+    # sockets would eliminate this, but Renode does not yet support them.
     with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
         s.bind(("127.0.0.1", 0))
         return s.getsockname()[1]
@@ -50,10 +55,7 @@ def _find_free_port() -> int:
 def _find_renode() -> str:
     path = shutil.which("renode")
     if path is None:
-        raise FileNotFoundError(
-            "renode executable not found in PATH. "
-            "Install Renode from https://renode.io/"
-        )
+        raise FileNotFoundError("renode executable not found in PATH. Install Renode from https://renode.io/")
     return path
 
 
@@ -70,10 +72,7 @@ async def flash(self, source, load_command: str | None = None):
         and resets the machine.
         """
         if load_command is not None and load_command not in _ALLOWED_LOAD_COMMANDS:
-            raise ValueError(
-                f"unsupported load_command {load_command!r}, "
-                f"allowed: {sorted(_ALLOWED_LOAD_COMMANDS)}"
-            )
+            raise ValueError(f"unsupported load_command {load_command!r}, allowed: {sorted(_ALLOWED_LOAD_COMMANDS)}")
 
         firmware_path = self.parent._tmp_dir.name + "/firmware"
         async with await FileWriteStream.from_path(firmware_path) as stream:
@@ -85,15 +84,13 @@ async def flash(self, source, load_command: str | None = None):
             cmd = load_command
         else:
             cmd = _detect_load_command(firmware_path)
-        self.parent._firmware_path = firmware_path
-        self.parent._load_command = cmd
+        self.parent.set_firmware(firmware_path, cmd)
 
-        if hasattr(self.parent.children["power"], "_process"):
-            monitor = self.parent.children["power"]._monitor
-            if monitor is not None:
-                await monitor.execute(f'{cmd} @"{firmware_path}"')
-                await monitor.execute("machine Reset")
-                self.logger.info("firmware hot-loaded and machine reset")
+        power: RenodePower = self.parent.children["power"]
+        if power.is_running:
+            await power.send_monitor_command(f'{cmd} @"{firmware_path}"')
+            await power.send_monitor_command("machine Reset")
+            self.logger.info("firmware hot-loaded and machine reset")
 
     @export
     async def dump(self, target, partition: str | None = None):
@@ -110,6 +107,21 @@ class RenodePower(PowerInterface, Driver):
     _process: Popen | None = field(init=False, default=None, repr=False)
     _monitor: RenodeMonitor | None = field(init=False, default=None, repr=False)
 
+    @property
+    def is_running(self) -> bool:
+        """Whether the Renode process is running with an active monitor."""
+        return self._process is not None and self._monitor is not None
+
+    async def send_monitor_command(self, command: str) -> str:
+        """Send a command to the Renode monitor.
+
+        Provides a public interface for sibling drivers to interact with
+        the monitor without accessing private attributes directly.
+        """
+        if self._monitor is None:
+            raise RuntimeError("Renode is not running")
+        return await self._monitor.execute(command)
+
     @export
     async def on(self) -> None:
         """Start Renode, connect monitor, configure platform, and begin simulation."""
@@ -135,37 +147,31 @@ async def on(self) -> None:
         self._monitor = RenodeMonitor()
         try:
             await self._monitor.connect("127.0.0.1", port)
-
-            machine = self.parent.machine_name
-            self._monitor.add_expected_prompt(machine)
-            await self._monitor.execute(f'mach create "{machine}"')
-            await self._monitor.execute(
-                f"machine LoadPlatformDescription @{self.parent.platform}"
-            )
-
-            pty_path = self.parent._pty
-            await self._monitor.execute(
-                f'emulation CreateUartPtyTerminal "term" "{pty_path}"'
-            )
-            await self._monitor.execute(
-                f"connector Connect {self.parent.uart} term"
-            )
-
-            for cmd in self.parent.extra_commands:
-                await self._monitor.execute(cmd)
-
-            if self.parent._firmware_path:
-                load_cmd = self.parent._load_command or "sysbus LoadELF"
-                await self._monitor.execute(
-                    f'{load_cmd} @"{self.parent._firmware_path}"'
-                )
-
+            await self._configure_simulation()
             await self._monitor.execute("start")
             self.logger.info("Renode simulation started")
         except Exception:
             await self.off()
             raise
 
+    async def _configure_simulation(self) -> None:
+        """Set up the machine, platform, UART, and firmware in the monitor."""
+        machine = self.parent.machine_name
+        self._monitor.add_expected_prompt(machine)
+        await self._monitor.execute(f'mach create "{machine}"')
+        await self._monitor.execute(f"machine LoadPlatformDescription @{self.parent.platform}")
+
+        pty_path = self.parent._pty
+        await self._monitor.execute(f'emulation CreateUartPtyTerminal "term" "{pty_path}"')
+        await self._monitor.execute(f"connector Connect {self.parent.uart} term")
+
+        for cmd in self.parent.extra_commands:
+            await self._monitor.execute(cmd)
+
+        if self.parent._firmware_path:
+            load_cmd = self.parent._load_command or "sysbus LoadELF"
+            await self._monitor.execute(f'{load_cmd} "{self.parent._firmware_path}"')
+
     @export
     async def off(self) -> None:
         """Stop simulation, disconnect monitor, and terminate the Renode process."""
@@ -181,12 +187,16 @@ async def off(self) -> None:
             await self._monitor.disconnect()
             self._monitor = None
 
-        self._process.terminate()
         try:
-            await to_thread.run_sync(self._process.wait, 5)
-        except TimeoutExpired:
-            self._process.kill()
-        self._process = None
+            self._process.terminate()
+            try:
+                await to_thread.run_sync(self._process.wait, 5)
+            except TimeoutExpired:
+                self._process.kill()
+        except ProcessLookupError:
+            pass
+        finally:
+            self._process = None
 
     @export
     async def read(self) -> AsyncGenerator[PowerReading, None]:
@@ -197,13 +207,18 @@ def close(self):
         """Synchronous cleanup for use during driver teardown."""
         if self._process is not None:
             if self._monitor is not None:
+                self._monitor.close_sync()
                 self._monitor = None
-            self._process.terminate()
             try:
-                self._process.wait(timeout=5)
-            except TimeoutExpired:
-                self._process.kill()
-            self._process = None
+                self._process.terminate()
+                try:
+                    self._process.wait(timeout=5)
+                except TimeoutExpired:
+                    self._process.kill()
+            except ProcessLookupError:
+                pass
+            finally:
+                self._process = None
 
 
 @dataclass(kw_only=True)
@@ -228,9 +243,7 @@ class Renode(Driver):
     extra_commands: list[str] = field(default_factory=list)
     allow_raw_monitor: bool = False
 
-    _tmp_dir: TemporaryDirectory = field(
-        init=False, default_factory=TemporaryDirectory
-    )
+    _tmp_dir: TemporaryDirectory = field(init=False, default_factory=TemporaryDirectory)
     _firmware_path: str | None = field(init=False, default=None)
     _load_command: str | None = field(init=False, default=None)
     _active_monitor_port: int = field(init=False, default=0)
@@ -248,6 +261,11 @@ def __post_init__(self):
         self.children["flasher"] = RenodeFlasher(parent=self)
         self.children["console"] = PySerial(url=self._pty, check_present=False)
 
+    def set_firmware(self, path: str, load_command: str) -> None:
+        """Set the firmware path and load command for the next power-on."""
+        self._firmware_path = path
+        self._load_command = load_command
+
     @property
     def _pty(self) -> str:
         return str(Path(self._tmp_dir.name) / "pty")
@@ -275,10 +293,7 @@ async def monitor_cmd(self, command: str) -> str:
         """
         if not self.allow_raw_monitor:
             raise RuntimeError(
-                "raw monitor access is disabled; "
-                "set allow_raw_monitor: true in exporter config to enable"
+                "raw monitor access is disabled; set allow_raw_monitor: true in exporter config to enable"
             )
         power: RenodePower = self.children["power"]
-        if power._monitor is None:
-            raise RuntimeError("Renode is not running")
-        return await power._monitor.execute(command)
+        return await power.send_monitor_command(command)