Repository navigation
fix(can_tools): fix can_diag.sh error-frame classification, restore, and lock/pause ownership - #14
Conversation
…shell CI Implements PR-C of Issue #9: - can_diag.sh: Diagnostic tool for CAN bus error capture * Pauses watchdog, enables error reporting, captures frames for N seconds (max 60) * Records: ip link show, sysfs counters, journalctl, candump error frames * Classifies errors by type (bit/form/stuff/position/ACK/PROT) * Restores settings via trap, documents bench/fixed-machine mode only * Falls back to inline flock/pause before lib A1 merge - .github/workflows/shellcheck.yml: GitHub Actions CI * Runs shellcheck -S warning on repo *.sh/*.bash and tests/ * Excludes: can-utils/, openimu/, and files modified in later PRs * Also runs tests/run.sh - tests/test_diag_classify.sh: Validates classification logic and output structure - tests/test_diag_restore.sh: Ensures settings restored on normal and error exits - tests/fakes/candump: Fake candump for testing All shell files pass shellcheck -S warning. All tests pass. Refs #9 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…d PR-A2/B files PR-C の workflow でcan_monitor.sh が警告を出すのは、 このファイルが PR-A2/PR-B で変更予定だからです。 checkout のパスの問題で除外が機能していなかったため、 basename で直接チェックするように修正しました。 Refs #9
…ocal - Remove fallback flock/pause implementation (can_diag must use lib to sync with watchdog) - can_diag.sh now exits with error if lib is not found - Move tests/fakes/candump to tests/diag_fakes/candump (test-local, not PR-A1 owned) - Update test paths to reference diag_fakes/candump - Clarify in code that divergent lock implementations would break mutual exclusion Refs #9
RyodoTanaka
left a comment
RyodoTanaka
left a comment
There was a problem hiding this comment.
レビュー（Opus）: PR-C can_diag.sh + shellcheck CI — 判定: 要修正
head 944971a、merge-base（develop 5247488）基準の diff。差分は 5 ファイルで、lib と tests/fakes には触れていない（範囲外の変更は無い）。bash tests/run.sh は 6/6 が緑。shellcheck 0.10 で -S warning をかけた結果は対象・fakes ともに 0 件、CI も緑。
ただしエラーフレーム分類は実際には一度も動いておらず、試験はそれを見逃している。復元も「元の値へ」になっていない。
クリティカル
C1. エラーフレームの解析が全く動かない（正規表現が不正）
can_diag.sh:149 の [[ "$line" =~ ^'('([^)]+')' ]] は括弧の対応が取れていない（^\(([^)]+\) になり、グループが閉じていない）。このため [[ ]] は rc=2（正規表現のコンパイル失敗）になり、どの行にもマッチしない。
$ line='(can0) C0000104#010A000010000000'; [[ "$line" =~ ^'('([^)]+')' ]]; echo $?
2
結果として、分類も、診断ファイルへのフレームの書き出しもされない。tests/run.sh の出力を見ても === CAN Error Frames === と === Error Frame Summary === の中身は空なのに、試験は通っている（C3）。
C2. 解析している形式が実際の candump の出力と合わない
candump -e any,0~0,#FFFFFFFF（-L無し）の実際の出力はcan0 20000088 [8] 00 00 80 19 00 08 00 00 ERRORFRAMEの形で、その後にタブ下げの解読行が続く。(iface) ID#DATAの形にはならない。-Lを付けた場合は(1697000000.123456) can0 20000088#0000801900080000で、先頭の括弧の中はタイムスタンプであり iface ではない。- 偽物の
tests/diag_fakes/candumpが出す(can0) C0000104#...は、どちらの形式でもない。can_id のC0000104もCAN_ERR_FLAG（0x20000000）を持たない（EFF|RTR のビットが立っている）。さらに偽物のデータは種別を data[0] に置いている（010A0000…）ので、data[2] を読む実装と食い違い、仮に正規表現が直っても全部unknownになる。
→candump -L -e …に固定し、^\(([0-9.]+)\) ([^ ]+) ([0-9A-Fa-f]+)#([0-9A-Fa-f]*)で読むこと。偽物も、実機から取った-Lの行（例:(…) can1 20000088#0000801900080000）にすること。
C3. エラーフレーム分類が linux/can/error.h と違う
- data[2]（
CAN_ERR_PROT_*）は 0x01 BIT / 0x02 FORM / 0x04 STUFF / 0x08 BIT0 / 0x10 BIT1 / 0x20 OVERLOAD / 0x40 ACTIVE / 0x80 TX。実装は 0x20 をack_error、0x40 をprot_errorとして扱っており、どちらも誤り。0x80（CAN_ERR_PROT_TX、送信中か受信中か）も見ていない。 - **
CAN_ERR_ACKは can_id 側のクラスビット（0x20）**で、data にはない。can_id のクラス（TX_TIMEOUT 0x1 / LOSTARB 0x2 / CRTL 0x4 / PROT 0x8 / TRX 0x10 / ACK 0x20 / BUSOFF 0x40 / BUSERROR 0x80 / RESTARTED 0x100 / CNT 0x200）を一切見ていないので、ACK 欠落・bus-off・controller の警告や passive（data[1]）・再起動を数えられない。E（エラーストームの原因、FD 起因の form error という仮説）の切り分けに必要な情報が取れない。 - data[3] は
CAN_ERR_PROT_LOC_*（エラーが起きたビットフィールドの位置コード。例: 0x08 = SOF、0x19 = ACK slot、0x18 = CRC delimiter）であり、「position」という数値ではない。名前に変換するか、16 進のまま出すこと。 ifを順に並べて後勝ちで上書きしているので、複数のビットが立っているフレームは最後の 1 種類にしか数えられない。各ビットを独立に数えること。error_details[$key]は最後の 1 件で上書きされる。
→ 試験には、既知の-L行（ACK 欠落20000020#…、bus-off20000040#…、20000088#0000801900080000（class PROT|BUSERROR、data[2]=0x80 TX、data[3]=0x19 ACK slot）、form error20000088#0000020000000000など）ごとの期待件数を入れること。
C4. 分類の試験が空振り
test_diag_classify.sh が見ているのは、見出しの文字列が 5 つあることと、berr=off・pause の削除・ロックの解放だけ。件数も分類結果も検査していないので、C1〜C3 があっても緑になる。ロックの検査 [[ ! -f lock || ! -f /proc/$$/fd/9 ]] は、試験のシェル自身の fd を見ているので常に真。
高
H1. 「元の設定」に戻していない
cleanup() は元の値を読まず、常に berr-reporting off・restart-ms ${CAN_RESTART_MS:-100}・bitrate で up し直しているだけ。fd・dbitrate・txqueuelen は指定していない。can_read_conf も呼んでいないので、conf の値も反映されない。
- 実測（偽物の ip、元の値
restart_ms=0 berr=off fd=on）: 実行中はrestart_ms=100 berr=on fd=off、SIGTERM の後はrestart_ms=100 berr=off fd=off。restart-ms が 0 から 100 に戻され、fd も失われた。 - PR-B で既定値が
restart-ms 0になった後にこの経路を通ると、診断のたびに upstream のバグ（echo_skb occupied → DQL Oops）の条件であるrestart-ms 100が再び有効になり、しかも復元後もそのまま残る。診断中に berr を on にするときも、restart-ms を 100 に変えてはならない。
→ 開始時にcan_iface_states >"$file"（プレーンなコマンドとして呼び、ファイル経由で読む。契約の追記どおり）を実行して、元の restart_ms・berr・fd・admin を保存し、加えて sysfs から txqueuelen を保存する。on にするときは berr だけを変え、それ以外は元の値を明示する。復元はその保存値で行う（CAN_BERR_REPORTING=<元> CAN_RESTART_MS=<元> CAN_FD=<元> CAN_TXQUEUELEN=<元> can_setup_iface "$if"を、サブシェルを作らずに呼べばよい）。元が admin down だった iface は down に戻すこと。
H2. 子プロセスに fd 9（ロック）が漏れ、強制終了の後もロックが残る
timeout/candump・ip・journalctl を直接起動しているので、fd 9 を継承する（§1 は「子には 9>&-」を求めている）。実測: SIGTERM と SIGHUP で can_diag を止めると、cleanup 自体は走るが、孤立した candump（timeout の子）が lock の fd を持ったまま最大 duration+1 秒動き続ける。その間、watchdog や setup はロックを取れない。直後にもう一度診断を走らせても、ロックの取得に失敗した。
→ timeout … candump … 9>&- とし、candump の PID を控えて cleanup で kill すること。ip は can_run_bounded を経由させる（プレーンなコマンドとして呼び、出力はファイルへ。$( )・< <( )・パイプの中で呼ばない）。
H3. ip の実行に上限が無い
ip link set … down/up・ip -j -d -s link show は時間の上限なしに直接呼ばれている。この障害で実際に起きた「rtnl が詰まる」状況では、can_diag がロックと pause を持ったまま止まり、60 s の上限が成り立たない（上限があるのは candump だけ）。H2 と同じく can_run_bounded（WD_RTNL_TIMEOUT_S）を使うこと。
中
M1. pause が必須になっていない（N7）
can_pause 2 || { echo WARNING; # Continue anyway } となっていて、pause に失敗しても berr を on にして続行する。中止して exit ≠ 0 にすること。
M2. 自分の物でない pause を消す
cleanup は無条件に can_unpause を呼ぶ。そのため次の 2 つが起きる。
(a) ロックの取得に失敗した 2 本目の can_diag が、実行中の 1 本目の pause を消す。
(b) 運用者が先に置いた pause（例: 30 分）を、診断の終わりに消す。
→ pause を置けた場合にだけ unpause する。既存の pause があれば、その期限を退避しておき、終わったら戻す。lock の取得が成功していないときは、ip の復元も unpause もしない。
M3. 手順の順序が §6 と違う
§6 は can_pause → can_lock の順。この実装は lock → pause。どちらでも成り立つが、仕様から外れるならその理由を PR に書くか、仕様に合わせること。
M4. 試験に強制終了の経路が無い
test_diag_restore.sh の「error exit」は不正な引数を渡す場合だけで、lib の読み込みより前に終わるため、何も変更されないまま終わる（空振り）。次の試験を足すこと。
- 終わらない candump を偽物で用意し、SIGTERM と SIGINT で止める。そのうえで元の値（restart_ms=0・fd=on を含む）へ戻ること、pause が解除されること、lock が解放されること（
flock -nで取れることで確かめる）、子が残らないことを検査する。 ipが hang するケース（hang/ip）で、上限内に終わること。- ロックの取得に失敗したときに、他人の pause を消さないこと。
M5. sysfs と ip の出力が試験で空
sysfs の読み取りが /sys/class/net/... に直書きされていて、CAN_TOOLS_SYSFS_ROOT を無視している（lib の can_read_stats を使うこと）。偽物の ip は -j にも -d -s link show にも対応していないので、出力が空になる。試験の出力でも can0: の下と IP Link Show の中身が空。lib・fakes は触らない方針なら、試験で sysfs のルートを渡して値が出ることを検査すること。
低
- L1.
-h/--helpの判定が引数の検証より後にあるので、--helpで Usage を出して rc=1 になり、ヘルプの本文に届かない。 - L2.
0秒が通る（ヘルプ上は 1〜60）。 - L3. ヘルプに "With caution in production environments" とあり、「ベンチ・機械停止中限定」と矛盾する。削ること。ヘルプの例にある
./tests/fakes/candumpは、今はtests/diag_fakes/candump。 - L4. 2 つ目の trap
"rm -f …; cleanup"では、cleanupの中のrc=$?が rm の 0 になる。 - L5.
berr-reporting onの失敗を|| trueで握りつぶしたまま採取を続ける。失敗した iface は診断ファイルに明記すること。 - L6. CI: 除外リストは §5 どおりでよい。ただしコメントは「later PRs (PR-A2, PR-B)」だけなので、「PR-D でこの除外を外す」と明記すること。拡張子の無い
tests/fakes/*・tests/diag_fakes/*は対象外になっている（§5 の「tests/」の範囲）。ローカルでは 0 件だったので、対象に含めて問題ない。shellcheck は ubuntu-latest に最初から入っており、apt はその予備として妥当。 - L7. 既存スクリプトの警告修正（§6 の C 欄）は、shellcheck 0.10 では警告 0 件だったので、変更不要で問題ない。PR 本文にその旨を書いておくとよい。
確認できた点（OK）
- lib の
can_lock/can_pause/can_unpause/can_unlockを source して使っている（watchdog と同じ機構）。lib が無ければ exit 1。 can_run_bounded系を$( )・< <( )・パイプの中で呼んでいない（そもそも呼んでいない。H2/H3 で導入するときは、プレーンなコマンド＋ファイル経由で呼ぶこと）。- 60 s の上限の検証と、
timeout duration+1による candump の上限。 - SIGTERM と SIGHUP で EXIT trap が走り、pause の削除・lock の解放・一時ファイルの削除は行われる（ただし H1 と H2 の問題がある）。
- 差分は範囲内（lib・
tests/fakesは無変更）。
必須: C1〜C4、H1〜H3、M1〜M2、M4。M3・M5・低は対応するか、理由を返信すること。
|
上のレビューの訂正 2 点（Opus）:
|
…iag-ci # Conflicts: # .github/workflows/shellcheck.yml
… (Issue #9 review) Addresses the PR-C review (C1-C4, H1-H3, M1-M5, low items): - Switch to `candump -L` and fix the line regex; the previous one failed to compile ([[ ]] rc=2), so no error frame was ever parsed or classified. - Reimplement classification strictly per linux/can/error.h: can_id class bits, data[1] CAN_ERR_CRTL_*, data[2] CAN_ERR_PROT_* (fixing the wrong 0x20/0x40 mapping and missing TX/OVERLOAD/ACTIVE), data[3] location (named via a CAN_ERR_PROT_LOC_* table instead of a raw number), data[4] CAN_ERR_TRX_*. Every set bit is counted independently instead of overwriting a single "last wins" classification. - Tests now assert exact per-category counts from known -L lines, not just that section headings exist. - H1: capture each interface's ORIGINAL restart-ms/berr/fd/admin via can_iface_states before flipping berr-reporting on, and restore exactly those values (plus conf's txqueuelen/dbitrate) on exit, instead of hardcoding restart-ms 100/berr off. Admin-down interfaces are left alone. - H2: close fd 9 for candump/journalctl, track the candump child, and kill it (TERM then KILL, plus `timeout --kill-after`) on exit so it cannot outlive can_diag.sh or hold the lock fd. - H3: the `ip -j -d -s link show` capture goes through can_run_bounded. - M1: a failed can_pause aborts with no changes made. - M2: only ever undo the pause can_diag.sh itself set; an already-active pause (another instance or an operator) has its exact deadline restored instead of being deleted or left clobbered. - M3: sequence is now pause -> lock, per Issue #9 §6. - M5: sysfs stats go through the lib's can_read_stats() (honors CAN_TOOLS_SYSFS_ROOT) instead of a hardcoded /sys/class/net path. - Low items: -h/--help checked before argument validation, duration 0 rejected, help text no longer contradicts bench/machine-stopped-only guidance, a failed berr-reporting-on is recorded per-interface instead of silently continuing, and the EXIT trap no longer lets an rm's rc clobber the script's own exit code. - tests/diag_fakes/candump (PR-C's own fake, not tests/fakes/) now emits realistic `-L` lines with documented expected per-category counts. - tests/test_diag_restore.sh gains SIGTERM/SIGINT forced-termination coverage, a lock-contention test for M2, and an `ip`-hang test for H3. can_tools_lib.bash and tests/fakes/ are untouched, per the PR-A1 contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RyodoTanaka
changed the title
|
対応状況（レビュー #14 、Issue #9 改訂4 §6 準拠、head
クリティカル
検証として、 高
中
低
試験・lint
|
RyodoTanaka
left a comment
RyodoTanaka
left a comment
There was a problem hiding this comment.
再レビュー（Opus）: head e218fb5 — 判定: 要修正（高 3 件・中 3 件）
この PR の差分は can_diag.sh・tests/diag_fakes/candump・tests/test_diag_{classify,restore}.sh の 4 ファイルで、lib と tests/fakes は変更していない。workflow を「触れない」形でこの PR から外した判断も妥当。
bash tests/run.sh は 6/6 が緑（diag 27 + 30）、shellcheck -S warning は 0 件。前回の指摘の大半は直っている。下に「前回指摘の確認」と「新しい指摘」を分けて書く。新しい指摘はどれも、手元で再現を確かめた。
前回指摘の確認
| 指摘 | 状態 |
|---|---|
C1 正規表現 / C2 -L の形式 |
解消。^\(([0-9]+(\.[0-9]+)?)\) ([^ ]+) ([0-9A-Fa-f]+)#([0-9A-Fa-f]*)$ は実機の candump -L の行に合い、CAN_ERR_FLAG が無ければ除外する。偽物も実機の形式になった |
| C3 分類 | 大部分は解消したが、残りがある（下の H1・M1）。class・data[1]・data[2]（0x20 OVERLOAD・0x40 ACTIVE・0x80 TX を含む）・data[4]、各ビットを独立に数える点は /usr/include/linux/can/error.h と一致した |
| C4 試験の空振り | 解消。件数を厳密に検査し、通常のフレームが混ざらないことも見ている |
| H1 元の値への復元 | 正常終了・SIGTERM・SIGINT・SIGHUP（candump が TERM を無視する場合も含む）で、restart_ms=0・fd=on・berr=off・admin=up へ戻ることを確かめた。txqueuelen と dbitrate は conf の値で戻る。ただし穴が 2 つある（下の H2・H3） |
| H2 fd 9 / 子プロセス | 解消。実行中の timeout と candump の /proc/<pid>/fd に lock は無かった。シグナルで止めた後も子プロセスは残らず、flock -n で lock を取れた |
| H3 ip の上限 | 解消。can_iface_states・can_run_bounded をプレーンなコマンドとして呼び、ファイル経由で読んでいる（契約どおり）。hang/ip の試験もある |
| M1 pause 必須 / M3 順序 | 解消（pause → lock、pause に失敗したら何も変えずに exit 1） |
| M2 自分の pause だけ | lock を取れなかった場合は直っている。ただし並行実行で穴がある（下の M2） |
| M4 強制終了の試験 / M5 sysfs | 解消。SIGINT の試験も本物だった（子の SigIgn は SIGQUIT だけで、rc=130 で終わる） |
| 低 L1〜L5 | 解消 |
新しい指摘
H1. CAN_ERR_PROT_LOC_*（data[3]）の対応表が 4 つ間違っている
/usr/include/linux/can/error.h の定義は次のとおり。
| 値 | 正しい名前 | 実装の名前 |
|---|---|---|
| 0x09 | RES0 | RES1 |
| 0x0A | DATA | RTR |
| 0x0D | RES1 | DATA |
| 0x0C | RTR | 欠落（UNKNOWN_0x0C） |
実測:
diag_classify_line "(1.0) can0 20000008#0000010A00000000" → loc=RTR （正しくは DATA）
diag_classify_line "(1.0) can0 20000208#0000020D00000000" → loc=DATA （正しくは RES1）
diag_classify_line "(1.0) can0 20000008#0000010C00000000" → loc=UNKNOWN_0x0C（正しくは RTR）
E（FD 起因の form error という仮説）の切り分けで、データ部分で起きたエラーが RTR と表示される。診断結果を取り違えることになるので「高」とした。偽物の candump と試験に、0x0A・0x0C・0x0D・0x09 の行を足すこと。
H2. berr を on にしている途中でシグナルを受けると、iface が down のまま残る。後から berr が on になり、pause も無い
_berr_ifaces+=("$_diag_if") が can_setup_iface の後にある。そのため、down から up の間に SIGTERM を受けると、その iface は復元の対象に入らない。
実測（berr-reporting on の up に 3 s かかる ip、1 s 後に SIGTERM）:
直後: can1=restart_ms=0 berr=off fd=on admin=down pause=none
6 s 後: can1=restart_ms=0 berr=on fd=on admin=up （孤立した ip の子が up を完了）
lock を手放し、pause も消えた状態で、can1 は「down のまま」になり、その後「berr on のまま」になる。さらに、仮に対象リストに入っていたとしても、cleanup の can_setup_iface → can_run_bounded は「前の子が生きている間は起動しない」（R2）により 124 で失敗する。
→ _berr_ifaces+= を can_setup_iface の前に移すこと。cleanup では、まず CAN_RUN_BOUNDED_PID が終わるのを上限付きで待つ（それでも終わらなければ、復元できなかった iface として CRIT を出す）。
H3. cleanup 中に 2 回目のシグナルを受けると、復元が途中で終わる
cleanup の中でも trap 'exit 143' TERM などが生きている。Ctrl-C の 2 度押しや、systemd の stop → kill がこれに当たる。2 回目で cleanup が中断される。
実測（restore の ip に 2 s かかる、TERM を 1 s 間隔で 2 回送る）:
can0=berr=on admin=up（復元されない） can1=admin=down → 後で up pause=残る lock=解放
→ cleanup の先頭で trap '' TERM INT HUP とする（cleanup 自体は bounded の ip で上限があるため、無視してよい）。2 回送る試験を足すこと。
M1. CAN_ERR_CNT の値が違う（0x1000000 → 正しくは 0x200）
can_diag.sh:129。error.h は #define CAN_ERR_CNT 0x00000200U。mcp251xfd はエラーフレームに CNT を付けて data[6]/data[7] に TEC/REC を入れるので、今のままでは一度も数えられない。CNT のときは data[6]/data[7] の最大値（あるいは最後の値）も要約に出すとよい（E の切り分けで error passive への接近が分かる）。
M2. 並行実行で pause が無いまま berr on になる
A の実行中に B を起動すると、次の順に進む。
- B は A の pause を「既存」として保存し、lock を待つ。
- A の cleanup が
can_unpause→can_unlockを実行する。 - B は lock を取り、pause ファイルが無いまま berr を on にして採取する。
- B の終了時に、A の古い期限を書き戻す。
実測で、B の採取中ずっと pause=NONE・berr=on だった（N7 違反）。
→ lock を取った後に can_pause を取り直す（または can_pause_active を確かめて、無ければ置き直す）。書き戻しは「ファイルの中身が自分の置いた期限のままのとき」だけにすること。
M3. 分類が lock・pause・berr on を持ったまま走り、しかも遅い
復元は cleanup で行われるので、candump の後の分類（1 行ごとに $(_diag_loc_name) / $(_diag_trx_name) を fork し、1 行ごとに診断ファイルを open して追記する）の間も、berr on・lock・pause が続く。
実測: PROT|CRTL の 2 万行で 13.5 s（x86）。RPi ではおよそ 4〜5 倍かかる。エラーストームが 60 s 続くと 10 万行を超え、数分かかる。その間に pause（2 分）が切れ、lock も握ったままになる。
→ candump が終わったらすぐに復元・unpause・unlock を行い、分類はその後で行う。名前の変換は配列か case の直書きで済ませ、fork しないこと。
低
- L1. 偽物の candump の通常フレームは
18DA31F9#…で問題ない。ただ、FD のフレーム（##）が混じっても落ちないことを 1 行足して確かめておくとよい（今の正規表現は FD の行を除外するので、挙動は正しい）。 - L2. data[4] の TRX で CANH と CANL が同時に立つ場合（例: 0x44）は UNKNOWN になる。nibble ごとに名前を付けると親切。
- L3. 診断ファイルと
candump_*.txtは/run（tmpfs）上で大きさに上限が無い。ストーム時に備えて、生の行の書き出しを先頭 N 行に限るなどの上限を設けるとよい。
必須: H1〜H3、M1〜M3（どれも小さな修正）。直したら差分に絞って再レビューする。
… M3 ordering/perf Second-review fixes (Issue #9, PR-C): - H1: CAN_ERR_PROT_LOC_* (data[3]) table had 4 wrong entries (0x09/0x0A/ 0x0C/0x0D) vs /usr/include/linux/can/error.h; corrected and covered with a unit test for every defined LOC code plus an UNKNOWN fallback case. - H2: register an interface for restore BEFORE calling can_setup_iface (berr on), not after - otherwise a signal arriving mid-call left it unrestored. cleanup now also waits (bounded by WD_RTNL_TIMEOUT_S) for an outstanding can_run_bounded child from an interrupted call before attempting to restore, instead of immediately getting 124'd by the "previous child still alive" guard. - H3: cleanup disables TERM/INT/HUP for the rest of its own lifetime (`trap '' TERM INT HUP`) so a second signal arriving while restore is still in progress cannot abort it partway through. - M1: CAN_ERR_CNT was 0x1000000, should be 0x200; now also tracks a per-iface TX/RX error-counter (data[6]/data[7]) high-water mark and reports it in the summary. - M2: the gap between the mandatory pre-lock can_pause probe and actually acquiring can_lock is not serialized, so a second instance could start, run its whole capture with berr-reporting on, and finish in that window with no pause active. Re-verify (and re-assert if needed) that a pause is genuinely active once the lock is actually held, and only restore/remove the pause file when it still holds exactly what we most recently wrote. - M3: restore/unpause/unlock now happen immediately after the candump capture finishes, BEFORE classification - not after - so a large capture no longer holds berr-reporting/the lock/the pause for longer than the capture itself. Classification itself is also now fork-free per line (results come back through a global instead of `$(...)` command substitution) and raw output is written through a single redirection instead of reopening the diag file per line; 20k lines classify in ~5s (vs 13.5s previously) and a full run with that much output now completes in ~7s end-to-end instead of minutes. - L1: added a CAN-FD line (`id##data`) regression test confirming it is excluded rather than mis-parsed. - L2: CAN_ERR_TRX_* (data[4]) now decodes CANH and CANL faults independently and reports both when set together (e.g. 0x44 -> CANH_NO_WIRE+CANL_NO_WIRE) instead of falling back to UNKNOWN. - L3: raw error-frame lines written to the diag file are capped (CAN_TOOLS_DIAG_MAX_RAW_LINES, default 5000) during a storm; the summary counts are always computed from the full capture regardless of the cap. - Fixed a `set -e` footgun introduced by the M3 rewrite: `(( x++ ))` evaluates to the PRE-increment value, so `(( raw_total++ ))` starting from 0 aborted the script under `set -e` on the first match. Changed to pre-increment (`(( ++x ))`) throughout. New tests: tests/test_diag_classify_units.sh (sources can_diag.sh directly to check every LOC code, TRX combos, CNT/TEC/REC, and FD-line exclusion without depending on the full capture pipeline or fakes), plus three new scenarios in tests/test_diag_restore.sh (SIGTERM mid berr-on setup, a second TERM during cleanup, two concurrent runs never sampling unpaused). Two new fakes (tests/diag_fakes/ip_slow_berr_on, ip_slow_berr_off) wrap tests/fakes/ip unmodified to inject a delay into a specific ip call for these signal-timing tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
can_now() now defaults this var internally (merged via develop's fix/can-tools-lib-errexit, PR #17), so can_diag.sh's own workaround for the unbound-variable trip under set -u is no longer needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
再レビュー（#14 、head 高H1. H2. berr-on 設定中のシグナルで iface が未復元になる
H3. 2 回目のシグナルで復元が中断される
中M1. M2. 並行実行で pause が無いまま berr on になる M3. 分類が lock/pause/berr on を保持したまま走る・遅い
この書き換えの過程で 低
試験・lintCI (shellcheck workflow, develop #16/#17 由来) も pass。
|
RyodoTanaka
left a comment
RyodoTanaka
left a comment
There was a problem hiding this comment.
再々レビュー（Opus）: e218fb5..2cf9047 — 判定: マージ可
PR の差分（merge-base 基準）は can_diag.sh・tests/diag_fakes/{candump,ip_slow_berr_on,ip_slow_berr_off}・tests/test_diag_{classify,classify_units,restore}.sh の 7 ファイルで、lib・fakes・他スクリプトへの変更は無い（develop を merge した分は除く）。bash tests/run.sh は 10/10 が通り（diag restore 43 件を含む）、shellcheck -S warning は 0 件だった。前回の再現手順（scratchpad の sigt/）で、各指摘が直ったことを確かめた。
| 前回 | 確認 |
|---|---|
| H1 LOC の対応表 | /usr/include/linux/can/error.h と全値が一致した。0x09 は RES0、0x0A は DATA、0x0C は RTR、0x0D は RES1 と出る。TRX は nibble ごとに読み、0x44 は CANH_NO_WIRE+CANL_NO_WIRE になる。FD の行（##）は除外される |
| H2 berr を on にしている途中の SIGTERM | 前回は can1 が down のまま残り、後から berr on になった。今回は、その直後も 5 s 後も can0・can1 とも restart_ms=0 berr=off fd=on admin=up で、pause は無く、lock も解放された。動いている bounded の子は上限付きで待ち、待ち切れなければ CRIT を出す |
| H3 2 回目のシグナル | TERM→TERM→INT を送っても、復元は最後まで行われた（pause は無く、lock も解放）。cleanup の先頭で trap '' TERM INT HUP を設定している |
| M1 CNT | 0x200 になった。tec_max/rec_max を data[6]/data[7] から取る（7F05 は tec=127 として、もう 1 行と合わせて rec_max=128 と出る） |
| M2 並行実行 | B の採取中、ずっと pause が有効だった（前回は NONE）。lock を取った後に can_pause_active を確かめ、無ければ置き直している |
| M3 | _diag_release は candump の直後、分類の前に呼ばれる。分類は fork しなくなり、2 万行で 13.5 s かかっていたものが 5.6 s になった。分類は解放の後なので、時間がかかっても安全性に影響しない |
| 低 | 生の行に上限（5000）、TRX の nibble、FD の行の除外。いずれも対応済み |
実装者からの質問への判断: A/B が交互に動いた後に A の pause が残る件
残ってよい（低、マージを止めない）。 理由は次のとおり。
- 残るのは A が置いた期限で、
can_pause 2によるため長くても約 2 分で切れる。実測では、B の終了時点で残りは約 110 s だった。 - 残った pause は watchdog を「何もしない」側に倒すだけで、安全側に働く。berr を on にしている間に pause が無い、という N7 違反は起きない。
- can_diag はベンチ・機械停止中に限る道具で、2 本を同時に走らせるのは運用の誤り。pause の期間中も watchdog は定期的に WARNING を出す（N7）。
- 残さないためには、pause の持ち主を区別する必要がある。pause ファイルは期限の数字だけの形式なので、これは lib の契約の変更になり、この PR の範囲を超える。運用者が置いた pause を消さないことの方が大事で、今の実装はそれを守っている。
任意（低）:
- 既存の pause を書き戻すときは、診断ファイルと stderr に
restored pre-existing pause deadline=<n>と 1 行出すとよい。 - PR-D の README に「can_diag の後、最大約 2 分 pause が残ることがある」と書くとよい。
- lock を取った後の
can_pause 2 || trueは、失敗したら中止する（exit 1）方が M1 と揃う。
pause の持ち主を区別する仕組みが必要なら、別 Issue（lib）にすること。
1 participant
変更点
PR-C:
can_diag.sh（CAN バス側エラーストームの切り分け用診断スクリプト）専用の PR。CI の shellcheck workflow は別 PR に分割するため、このブランチからは外した
（
.github/workflows/shellcheck.ymlはdevelopに別途マージ済み（#16）。git fetch && git merge origin/develop後、この PR とその workflow の内容がbyte-identical になったため、この PR の diff には既に含まれていない）。
can_diag.sh
can_pause→can_lock→ berr-reporting を一時的に有効化 → N 秒間（最大 60 秒）キャプチャ → 必ず元の設定に戻す →
can_unpause（§6 の順序、trap で保証）。ip -j -d -s link show（can_run_bounded経由）、sysfs カウンタ（lib の
can_read_stats()経由、CAN_TOOLS_SYSFS_ROOTに従う）、journalctl -k末尾、candump -L -e any,0~0,#FFFFFFFFによるエラーフレームキャプチャ・
linux/can/error.hに正確に従った分類。Issue #9 のレビュー指摘への対応（C1〜C4, H1〜H3, M1〜M5, 低）
candump -Lの行を読む正規表現が不正でコンパイル不能（[[ ]]rc=2）だったため、分類が一度も動いていなかった。修正して実際にマッチすることを確認。
candump -L出力に合わせて固定。偽物（
tests/diag_fakes/candump、PR-A1 のtests/fakes/とは別物）も実機形式に。linux/can/error.hどおりに書き直し（can_id のクラスビット、data[1] CAN_ERR_CRTL_、data[2] CAN_ERR_PROT_（0x20/0x40 の誤りと
OVERLOAD/ACTIVE/TX の欠落を修正）、data[3] の位置コードを名前に変換、
data[4] CAN_ERR_TRX_*）。複数ビットは全部独立に数える（上書きしない）。
-L行ごとの期待件数を厳密に検査する（セクション見出しの有無だけでは合格しない）。
can_iface_statesで各 iface の元の restart-ms/berr/fd/adminを保存し、終了時にその値（conf の txqueuelen/dbitrate と合わせ）へ正確に復元。
admin down の iface には触れない。
9>&-）。candump の PID を追跡し、timeout --kill-afterと cleanup 側の TERM→KILL エスカレーションで、シグナル終了時も子プロセスを確実に止める。
ip -j -d -s link showをcan_run_bounded経由にして上限を効かせる。あれば、その期限を退避して終了時に正確に戻す（消さない・延長したままにしない）。
can_read_stats()経由にし、CAN_TOOLS_SYSFS_ROOTを尊重する。-h/--helpを引数検証より先に判定、0 秒を拒否、ヘルプの「ベンチ・機械停止中限定」と矛盾する文言を削除・例のパスを修正、
berr-reporting on の失敗を診断ファイルに明記して継続、EXIT trap の二重化で
rc が rm の戻り値に上書きされていた問題を解消。
can_tools_lib.bashとtests/fakes/は変更していない（PR-A1 の契約どおり）。テストファイル
tests/test_diag_classify.sh: C1〜C4・M5 を検証（既知の-L行に対する正確なカテゴリ別件数、ordinary フレームの除外、sysfs 実値の反映）。
tests/test_diag_restore.sh: H1（元の restart-ms/berr/fd への復元）、H2/M4（SIGTERM・SIGINT による強制終了でも復元・candump 停止・ロック解放）、
M1/M2（pause 必須・他インスタンスの pause を壊さない）、H3（
ipがハングしても上限内に終わる）。
tests/diag_fakes/candump: PR-C 専用の偽物（tests/fakes/とは別）。実機形式の-L行と、期待件数をコメントで明記。意図的に壊して確認
分類のビット対応、H1 の復元値、M2 の pause 退避、H2 の子プロセス停止を個別に
壊した上で
bash tests/run.shが正しく失敗することを確認済み（4 パターン）。試験結果
shellcheck:
Refs #9
🤖 Generated with Claude Code