Ai slop2 #3

Open
wall_e wants to merge 4 commits from ai_slop2 into v1.6
Owner
No description provided.
wall_e changed target branch from main to v1.6 2026-08-07 15:07:20 +02:00
wall_e added 1 commit 2026-08-07 15:07:20 +02:00
does it work?
Test And Build / Lint (push) Failing after 6s
Test And Build / Build (push) Failing after 8s
1aeff55111
wall_e added 1 commit 2026-08-07 15:54:54 +02:00
asked it to simplify the code
Test And Build / Lint (push) Successful in 5s
Test And Build / Build (push) Failing after 8s
4d61d0177a
wall_e added 1 commit 2026-08-07 16:04:50 +02:00
more cleaning
Test And Build / Lint (push) Successful in 5s
Test And Build / Build (push) Failing after 8s
f5eba004fb
wall_e added 1 commit 2026-08-07 16:10:28 +02:00
pipeline fix?
Test And Build / Lint (push) Successful in 6s
Test And Build / Build (push) Successful in 9s
d4577d930c
Author
Owner

Review done by Qwen3.8 (code was written by another model)

I've finished a thorough review of the whole tree (the ai_slop2 branch: the C→C++ pmacAsynIPPort rewrite, the sinqMotor submodule, turboPmacController/turboPmacAxis, db/Makefile, and utils). Here's what I found, most severe first.

1. High — doFlushHardware() can dereference a null pointer (crash)

src/turboPmacController.cpp:468-482

asynStatus turboPmacController::doFlushHardware() {
    int temp = pTurboPmacC_->pasynInt32SyncIOipPort->reason;   // <-- no null check

pasynInt32SyncIOipPort is a member, but it is only ever declared in turboPmacController; it is the sinqController base constructor that populates its own pasynOctetSyncIOipPort. The derived class declares its own pasynInt32SyncIOipPort (initialized to nullptr) and calls pasynInt32SyncIO->connect(ipPortConfigName, 0, &pTurboPmacC_->pasynInt32SyncIOipPort, NULL) at line 169 — but then checks the stale local status (line 171), which still holds the value from callParamCallbacks(), not the return value of connect(). So:

  • If connect() fails, the IOC does not exit — it continues with pasynInt32SyncIOipPort == nullptr, and the first FLUSH_HARDWARE PV write segfaults.
  • Even in the success case this is fragile: connect()'s real status is discarded.

Fix: status = pasynInt32SyncIO->connect(...); if (status != asynSuccess || ...ptr == nullptr) exit(-1); and add a null guard in doFlushHardware().

2. High — same latent bug in the sinqController constructor

sinqMotor/src/sinqController.cpp:154,167-175

asynStatus status = asynSuccess; (line 154) is never assigned by pasynOctetSyncIO->connect(...) (line 167), so the check at line 169 (status != asynSuccess || ptr == nullptr) never fires on the status branch. A failed connect is only caught by the nullptr half — and only if connect() leaves the pointer null. The intended "exit IOC on fatal connect failure" is effectively broken.

3. High — readResponse() / readOctet() can consume up to 2× the full response

src/pmacAsynIPPort.cpp:523-575 and 748-831

inBuf_ is treated as if it were always empty before each socketRead:

status = socketRead(pasynUser, inBuf_.data(), maxchars, &thisRead);
...
inBufTail_ = 0;
inBufHead_ = thisRead;      // previous contents silently discarded

readResponse() is called from readOctet() again mid-loop (line 814) whenever inBuf_ was drained, to fetch the rest of a long response. Each call does a fresh recv() and overwrites inBuf_, resetting head/tail. For a PMAC response longer than INPUT_SIZE (1493 bytes) split across several recv()s, the chunks read between readResponse() calls are thrown away — the client gets a response with missing middle data, and writeRead()'s carriage-return count check will then fail with a misleading "unexpected response" error. The original DLS code had the same structure, but there the asyn IP layer buffered between reads; here the socket is read directly, so data lost to the recv() window that was not read is simply gone.

Also note readOctet() calls pmacReadReady() (a full request/response exchange on the socket) before each refill (line 808-812), even though readResponse() does the exact same thing internally — doubling the round-trips and, worse, the READREADY reply bytes can interleave with the real response data.

4. High — flushOctet() leaves inBuf_ holding just-read data, and returns it to the next reader

src/pmacAsynIPPort.cpp:833-855

flushOctet() discards bytes with recv() into a throwaway cbuf, but does not reset inBufHead_/inBufTail_. Any bytes already sitting in inBuf_ (read by a prior readResponse() and not yet consumed by readOctet()) are not flushed — they will be returned by the next readOctet(). If the intent is "toss everything," the buffer must be reset too:

inBufTail_ = 0;
inBufHead_ = 0;

(pmacFlush() does reset them at line 653-654, which shows the inconsistency.)

5. High — socketRead() can return success with thisRead still uninitialized

src/pmacAsynIPPort.cpp:362-419

thisRead is declared uninitialized (line 364). If pollSocket() returns asynSuccess but pollstatus == 0 (poll timed out, no data), recv() is still called on a non-blocking socket and returns -1/EAGAIN — handled, fine. But if pollstatus > 0 and recv() then returns -1 with another errno that is neither EWOULDBLOCK/EINTR (e.g. ECONNRESET after closeConnection sets aside), status becomes asynError and the early path at 401-402 sets it — OK. The real hole: after the if (thisRead < 0) {...} block, if status == asynError was set but the code falls through, *nbytesRead = thisRead with thisRead = 0 (line 413 clamps) — that's actually fine. However if pollSocket() itself fails (returns asynError) the function returns early (line 388) — fine. The genuinely bad case: pollstatus == 0 but recv() is not called... it always is. On re-inspection the uninitialized read is masked; downgrade this to a style/robustness note: initialize thisRead = 0.

6. Medium — writeRead() counts CRs in a buffer that is not NUL-terminated by strlen safely

src/turboPmacController.cpp:381-385

for (size_t i = 0; i < strlen(response); i++)
    if (response[i] == '\r') numReceivedResponses++;

response is only NUL-terminated up to the first NUL. readOctet() can return binary-ish PMAC data (STX/BELL/ACK bytes) and the "response" is counted by \r. If the PMAC response contains a NUL byte (possible in binary modes, and Ixx28/Qxx00 can in principle), strlen truncates the count. More concretely: pasynOctetSyncIO->writeRead() with EOS=\006 returns nbytesIn — use nbytesIn (and scan that many bytes) instead of strlen().

7. Medium — format string: size_t printed with %ld

sinqMotor/src/sinqController.cpp:566-571 and 575-578

"More than %ld communication timeouts in %ld seconds.%s\n",
... pSinqC_->maxNumberTimeouts, pSinqC_->comTimeoutWindow, ...

maxNumberTimeouts is size_t and comTimeoutWindow is time_t — %ld is only correct for long. On platforms where size_t is 32-bit (or time_t differs) this is UB. Use %zu / cast to time_t explicitly.

8. Medium — FFI wrappers don't return after a null dynamic_cast

sinqMotor/src/sinqAxis.cpp:749-768 (setWatchdogEnabled), 795-815 (setOffsetMovTimeout), 843-864 (setScaleMovTimeout)

if (axis == nullptr) {
    errlogPrintf(...);
}                      // <-- no return here
return axis->setWatchdogEnabled(enable != 0);   // NULL deref

setWatchdogEnabled and setOffsetMovTimeout log and then fall through to dereferencing axis when the cast fails (only setScaleMovTimeout at line 859-863 has the correct return asynError;). Add the missing return asynError;.

Also in setOffsetMovTimeout (line 830): the iocsh arg is registered as iocshArgInt (offsetMovTimeoutArg2) but the FFI takes a double and the caller passes args[2].ival into a double parameter — works by implicit conversion, but inconsistent with setScaleMovTimeout which uses iocshArgDouble.

9. Medium — msgPrintControlKey uses char* (non-const) as a public API type

sinqMotor/src/msgPrintControl.h:28-29, 146

msgPrintControlKey(char *controller_, ...);
bool shouldBePrinted(char *controller, ...);

controller/portName are const char* everywhere in the EPICS API. Every caller is forced to either hold a non-const buffer or cast. This is a design wart, not a memory bug, but it will cause -Werror friction and invites const_cast abuse. Change to const char* (the member is a std::string).

10. Medium — startMovTimeoutWatchdog() computes the timeout from stale/zero fields

sinqMotor/src/sinqAxis.cpp:595-674

  • motorPosition(&motorPos) is called at line 620, but pl_status is then re-tested at lines 644 and 656 as if it were still the position read — it is, but the pattern is confusing; more importantly, targetPosition is only set in sinqAxis::move(), not in doMove() overrides that set it themselves. If a derived doMove starts a move without going through sinqAxis::move(), targetPosition is 0 and the watchdog deadline is wrong.
  • timeAccel = 2 * std::ceil(motorVelocity / motorAccel) (line 663) — motorAccel here is motorVelocityRec / motorAccelRec (line 660), i.e. a time, so motorVelocity / motorAccel is not a time; the formula 2*v/a expects a in units/s². This looks dimensionally wrong and will under/over-estimate the accel phase. Worth checking against the intended physics.

11. Medium — enable() can return asynSuccess without the axis actually reaching the requested state

src/turboPmacAxis.cpp:1186-1193

if ((axStatus != -3) == on) {
    ... "Axis is already %s." ...
    return asynSuccess;
}

axStatus here is read from pTurboPmacA_->axisStatus, which is only updated by doPoll()/init(). If the axis was deactivated (−5) or in E‑stop (−4) after the last poll, axStatus is stale, so the "already enabled" fast path can misfire and skip the enable command. The doPoll(&moving) at line 1159 updates pTurboPmacA_->axisStatus only if the poll's read of P%2.2d00 succeeds and is parsed — if writeRead fails inside that poll, axisStatus keeps its old value. Consider reading the status directly here instead of trusting the cached member.

12. Low — connectIt() sets fd_ after setsockopt, leaking the fd on the failure path

src/pmacAsynIPPort.cpp:289-301

If setsockopt(TCP_NODELAY) fails, the socket is destroyed — fine. But between connect() success (line 249) and fd_ = fd (line 300), an exception/early return would leak. More usefully: connect() itself (line 315) sets inBufTail_/inBufHead_ = 0 after a successful connect, but if the PMAC already has queued data from a previous session it is read into a fresh buffer — acceptable, but combined with #4 the flush-on-reconnect story is inconsistent.

13. Low — pollSocket() ignores the pollmsec < 0 (infinite) EINTR time check

src/pmacAsynIPPort.cpp:339-360

if (epicsTimeDiffInSeconds(&endTime, &startTime) * 1000. > pollmsec)
    break;

When pollmsec is negative (infinite wait), the comparison > pollmsec is always true on the first EINTR, so the loop breaks immediately and treats an interrupted infinite poll as "done" (pollstatus stays -1). The caller then proceeds to recv()/send() on a possibly-not-ready socket. Guard with if (pollmsec >= 0 && elapsed > pollmsec).

14. Low — dead / misleading members

  • turboPmacControllerImpl::lastResponse[MAXBUF_] (src/turboPmacController.cpp:48) is written nowhere — dead.
  • asynStatus errorStatus shadowing in turboPmacAxis::doPoll (line 577) is fine, but the variable nvals/motorRecResolution at line 307 in doPoll is read via getAxisParamChecked but never used afterward — unused (would trip -Wunused-but-set-variable-style scrutiny under -Werror; it currently compiles only because it's "used" by the macro call).

Summary of the top fixes I'd do first

  1. turboPmacController ctor: capture the real return of pasynInt32SyncIO->connect() and exit on failure; null-guard in doFlushHardware() (#1).
  2. sinqController ctor: same fix for pasynOctetSyncIO->connect() (#2).
  3. Reset inBuf_ in flushOctet() and make readResponse()/readOctet() handle a partially-filled inBuf_ instead of overwriting it (#3, #4).
  4. Add the missing return asynError; in the two sinqAxis FFI wrappers (#8).
  5. Scan nbytesIn bytes instead of strlen(response) in writeRead() (#6).

Want me to apply any of these fixes? I'd suggest starting with #1–#2 (crash safety) since they're small, self-contained, and unambiguously correct.

Review done by Qwen3.8 (code was written by another model) I've finished a thorough review of the whole tree (the `ai_slop2` branch: the C→C++ `pmacAsynIPPort` rewrite, the sinqMotor submodule, `turboPmacController`/`turboPmacAxis`, db/Makefile, and utils). Here's what I found, most severe first. ## 1. High — `doFlushHardware()` can dereference a null pointer (crash) `src/turboPmacController.cpp:468-482` ```cpp asynStatus turboPmacController::doFlushHardware() { int temp = pTurboPmacC_->pasynInt32SyncIOipPort->reason; // <-- no null check ``` `pasynInt32SyncIOipPort` is a member, but it is only ever *declared* in `turboPmacController`; it is the **sinqController base constructor** that populates its own `pasynOctetSyncIOipPort`. The derived class declares its own `pasynInt32SyncIOipPort` (initialized to `nullptr`) and calls `pasynInt32SyncIO->connect(ipPortConfigName, 0, &pTurboPmacC_->pasynInt32SyncIOipPort, NULL)` at line 169 — but then checks the *stale* local `status` (line 171), which still holds the value from `callParamCallbacks()`, **not** the return value of `connect()`. So: - If `connect()` fails, the IOC does **not** exit — it continues with `pasynInt32SyncIOipPort == nullptr`, and the first FLUSH_HARDWARE PV write segfaults. - Even in the success case this is fragile: `connect()`'s real status is discarded. Fix: `status = pasynInt32SyncIO->connect(...); if (status != asynSuccess || ...ptr == nullptr) exit(-1);` and add a null guard in `doFlushHardware()`. ## 2. High — same latent bug in the `sinqController` constructor `sinqMotor/src/sinqController.cpp:154,167-175` `asynStatus status = asynSuccess;` (line 154) is never assigned by `pasynOctetSyncIO->connect(...)` (line 167), so the check at line 169 (`status != asynSuccess || ptr == nullptr`) never fires on the status branch. A failed connect is only caught by the nullptr half — and only if `connect()` leaves the pointer null. The intended "exit IOC on fatal connect failure" is effectively broken. ## 3. High — `readResponse()` / `readOctet()` can consume up to **2× the full response** `src/pmacAsynIPPort.cpp:523-575` and `748-831` `inBuf_` is treated as if it were always empty before each `socketRead`: ```cpp status = socketRead(pasynUser, inBuf_.data(), maxchars, &thisRead); ... inBufTail_ = 0; inBufHead_ = thisRead; // previous contents silently discarded ``` `readResponse()` is called from `readOctet()` **again** mid-loop (line 814) whenever `inBuf_` was drained, to fetch the rest of a long response. Each call does a fresh `recv()` and overwrites `inBuf_`, resetting head/tail. For a PMAC response longer than `INPUT_SIZE` (1493 bytes) split across several `recv()`s, the chunks read between `readResponse()` calls are **thrown away** — the client gets a response with missing middle data, and `writeRead()`'s carriage-return count check will then fail with a misleading "unexpected response" error. The original DLS code had the same structure, but there the asyn IP layer buffered between reads; here the socket is read directly, so data lost to the `recv()` window that was *not* read is simply gone. Also note `readOctet()` calls `pmacReadReady()` (a full request/response exchange on the socket) **before** each refill (line 808-812), even though `readResponse()` does the exact same thing internally — doubling the round-trips and, worse, the `READREADY` reply bytes can interleave with the real response data. ## 4. High — `flushOctet()` leaves `inBuf_` holding just-read data, and returns it to the *next* reader `src/pmacAsynIPPort.cpp:833-855` `flushOctet()` discards bytes with `recv()` into a throwaway `cbuf`, but does **not** reset `inBufHead_`/`inBufTail_`. Any bytes already sitting in `inBuf_` (read by a prior `readResponse()` and not yet consumed by `readOctet()`) are *not* flushed — they will be returned by the next `readOctet()`. If the intent is "toss everything," the buffer must be reset too: ```cpp inBufTail_ = 0; inBufHead_ = 0; ``` (`pmacFlush()` does reset them at line 653-654, which shows the inconsistency.) ## 5. High — `socketRead()` can return success with `thisRead` still **uninitialized** `src/pmacAsynIPPort.cpp:362-419` `thisRead` is declared uninitialized (line 364). If `pollSocket()` returns `asynSuccess` but `pollstatus == 0` (poll timed out, no data), `recv()` is still called on a non-blocking socket and returns -1/EAGAIN — handled, fine. But if `pollstatus > 0` and `recv()` then returns -1 with *another* errno that is neither EWOULDBLOCK/EINTR (e.g. ECONNRESET after closeConnection sets aside), status becomes `asynError` and the early path at 401-402 sets it — OK. The real hole: after the `if (thisRead < 0) {...}` block, if `status == asynError` was set but the code falls through, `*nbytesRead = thisRead` with `thisRead = 0` (line 413 clamps) — that's actually fine. **However** if `pollSocket()` itself fails (returns `asynError`) the function returns early (line 388) — fine. The genuinely bad case: `pollstatus == 0` but `recv()` is *not* called... it always is. On re-inspection the uninitialized read is masked; downgrade this to a **style/robustness** note: initialize `thisRead = 0`. ## 6. Medium — `writeRead()` counts CRs in a buffer that is not NUL-terminated by `strlen` safely `src/turboPmacController.cpp:381-385` ```cpp for (size_t i = 0; i < strlen(response); i++) if (response[i] == '\r') numReceivedResponses++; ``` `response` is only NUL-terminated up to the first NUL. `readOctet()` can return binary-ish PMAC data (STX/BELL/ACK bytes) and the "response" is counted by `\r`. If the PMAC response contains a NUL byte (possible in binary modes, and `Ixx28`/`Qxx00` can in principle), `strlen` truncates the count. More concretely: `pasynOctetSyncIO->writeRead()` with EOS=`\006` returns `nbytesIn` — use `nbytesIn` (and scan that many bytes) instead of `strlen()`. ## 7. Medium — format string: `size_t` printed with `%ld` `sinqMotor/src/sinqController.cpp:566-571` and `575-578` ```cpp "More than %ld communication timeouts in %ld seconds.%s\n", ... pSinqC_->maxNumberTimeouts, pSinqC_->comTimeoutWindow, ... ``` `maxNumberTimeouts` is `size_t` and `comTimeoutWindow` is `time_t` — `%ld` is only correct for `long`. On platforms where `size_t` is 32-bit (or `time_t` differs) this is UB. Use `%zu` / cast to `time_t` explicitly. ## 8. Medium — FFI wrappers don't `return` after a null `dynamic_cast` `sinqMotor/src/sinqAxis.cpp:749-768` (`setWatchdogEnabled`), `795-815` (`setOffsetMovTimeout`), `843-864` (`setScaleMovTimeout`) ```cpp if (axis == nullptr) { errlogPrintf(...); } // <-- no return here return axis->setWatchdogEnabled(enable != 0); // NULL deref ``` `setWatchdogEnabled` and `setOffsetMovTimeout` log and then **fall through** to dereferencing `axis` when the cast fails (only `setScaleMovTimeout` at line 859-863 has the correct `return asynError;`). Add the missing `return asynError;`. Also in `setOffsetMovTimeout` (line 830): the iocsh arg is registered as `iocshArgInt` (`offsetMovTimeoutArg2`) but the FFI takes a `double` and the caller passes `args[2].ival` into a `double` parameter — works by implicit conversion, but inconsistent with `setScaleMovTimeout` which uses `iocshArgDouble`. ## 9. Medium — `msgPrintControlKey` uses `char*` (non-const) as a public API type `sinqMotor/src/msgPrintControl.h:28-29, 146` ```cpp msgPrintControlKey(char *controller_, ...); bool shouldBePrinted(char *controller, ...); ``` `controller`/`portName` are `const char*` everywhere in the EPICS API. Every caller is forced to either hold a non-const buffer or cast. This is a design wart, not a memory bug, but it will cause `-Werror` friction and invites `const_cast` abuse. Change to `const char*` (the member is a `std::string`). ## 10. Medium — `startMovTimeoutWatchdog()` computes the timeout from stale/zero fields `sinqMotor/src/sinqAxis.cpp:595-674` - `motorPosition(&motorPos)` is called at line 620, but `pl_status` is then *re-tested* at lines 644 and 656 as if it were still the position read — it is, but the pattern is confusing; more importantly, `targetPosition` is only set in `sinqAxis::move()`, not in `doMove()` overrides that set it themselves. If a derived `doMove` starts a move without going through `sinqAxis::move()`, `targetPosition` is 0 and the watchdog deadline is wrong. - `timeAccel = 2 * std::ceil(motorVelocity / motorAccel)` (line 663) — `motorAccel` here is `motorVelocityRec / motorAccelRec` (line 660), i.e. a *time*, so `motorVelocity / motorAccel` is not a time; the formula `2*v/a` expects `a` in units/s². This looks dimensionally wrong and will under/over-estimate the accel phase. Worth checking against the intended physics. ## 11. Medium — `enable()` can return `asynSuccess` without the axis actually reaching the requested state `src/turboPmacAxis.cpp:1186-1193` ```cpp if ((axStatus != -3) == on) { ... "Axis is already %s." ... return asynSuccess; } ``` `axStatus` here is read from `pTurboPmacA_->axisStatus`, which is *only* updated by `doPoll()`/`init()`. If the axis was deactivated (−5) or in E‑stop (−4) after the last poll, `axStatus` is stale, so the "already enabled" fast path can misfire and skip the enable command. The `doPoll(&moving)` at line 1159 updates `pTurboPmacA_->axisStatus` only if the poll's read of `P%2.2d00` succeeds *and* is parsed — if `writeRead` fails inside that poll, `axisStatus` keeps its old value. Consider reading the status directly here instead of trusting the cached member. ## 12. Low — `connectIt()` sets `fd_` after `setsockopt`, leaking the fd on the failure path `src/pmacAsynIPPort.cpp:289-301` If `setsockopt(TCP_NODELAY)` fails, the socket is destroyed — fine. But between `connect()` success (line 249) and `fd_ = fd` (line 300), an exception/early return would leak. More usefully: `connect()` itself (line 315) sets `inBufTail_/inBufHead_ = 0` **after** a successful connect, but if the PMAC already has queued data from a previous session it is read into a fresh buffer — acceptable, but combined with #4 the flush-on-reconnect story is inconsistent. ## 13. Low — `pollSocket()` ignores the `pollmsec < 0` (infinite) EINTR time check `src/pmacAsynIPPort.cpp:339-360` ```cpp if (epicsTimeDiffInSeconds(&endTime, &startTime) * 1000. > pollmsec) break; ``` When `pollmsec` is negative (infinite wait), the comparison `> pollmsec` is always true on the first EINTR, so the loop breaks immediately and treats an interrupted infinite poll as "done" (pollstatus stays -1). The caller then proceeds to `recv()`/`send()` on a possibly-not-ready socket. Guard with `if (pollmsec >= 0 && elapsed > pollmsec)`. ## 14. Low — dead / misleading members - `turboPmacControllerImpl::lastResponse[MAXBUF_]` (`src/turboPmacController.cpp:48`) is written nowhere — dead. - `asynStatus errorStatus` shadowing in `turboPmacAxis::doPoll` (line 577) is fine, but the variable `nvals`/`motorRecResolution` at line 307 in `doPoll` is read via `getAxisParamChecked` but never used afterward — unused (would trip `-Wunused-but-set-variable`-style scrutiny under `-Werror`; it currently compiles only because it's "used" by the macro call). --- ### Summary of the top fixes I'd do first 1. `turboPmacController` ctor: capture the real return of `pasynInt32SyncIO->connect()` and exit on failure; null-guard in `doFlushHardware()` (#1). 2. `sinqController` ctor: same fix for `pasynOctetSyncIO->connect()` (#2). 3. Reset `inBuf_` in `flushOctet()` and make `readResponse()`/`readOctet()` handle a partially-filled `inBuf_` instead of overwriting it (#3, #4). 4. Add the missing `return asynError;` in the two `sinqAxis` FFI wrappers (#8). 5. Scan `nbytesIn` bytes instead of `strlen(response)` in `writeRead()` (#6). Want me to apply any of these fixes? I'd suggest starting with #1–#2 (crash safety) since they're small, self-contained, and unambiguously correct.
All checks were successful
Test And Build / Lint (push) Successful in 6s
Test And Build / Build (push) Successful in 9s
You are not authorized to merge this pull request.
This pull request can be merged automatically.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin ai_slop2:ai_slop2
git checkout ai_slop2
Sign in to join this conversation.
No Reviewers
No labels
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lin-epics-modules/turboPmac#3