Terry Wilson via dev <[email protected]> writes: > When set_condition() queues a new condition while other tables already > have conditions in flight, it must return a sequence number that accounts > for those in-flight requests. The check for "any table has a requested > condition" referenced the bound method t.condition_state.request instead > of the .requested property, so the expression was always truthy and the > returned expected seqno could be one too high. > > Test against the .requested property (the in-flight condition slot) being > populated instead. > > Assisted-by: Claude Opus 4.8 <[email protected]> > Signed-off-by: Terry Wilson <[email protected]> > ---
This is an AI generated review of your patch. A human has looked at the results and deemed any concerns as plausible. Commit summary: single-line change in `python/ovs/db/idl.py`, inside `Idl.cond_change()` (lines 688-717). The commit fixes the "any table has an in-flight condition" check, which previously referenced the bound method `ConditionState.request` (always truthy) instead of the `requested` property, so the expected sequence number was one too high whenever no conditions were actually in flight. I verified the supporting pieces: - `ConditionState` (lines 91-136): `_req_cond` starts as `None`; `request()` moves `_new_cond` into `_req_cond`; `ack()` moves it back to `_ack_cond` and clears `_req_cond`. So `requested is not None` is the correct test for "a monitor_cond_change containing this table's condition is outstanding." - `compose_cond_change()` (lines 657-678) calls `table.condition_state.request()` only when actually composing a `monitor_cond_change`, and `send_cond_change()` (680-686) only composes when connected with no outstanding request, so `requested != None` does imply an outstanding round trip. - The reply path (lines 614-620) calls `ack_conditions()`, then `send_cond_change()`, then increments `cond_seqno` once per reply, which matches the arithmetic: a freshly-init'd condition needs one more ack if nothing is in flight (+1), or two (+2) when a request is already outstanding. - Test helper `update_condition()` in tests/test-ovsdb.py (lines 730-755) calls `cond_change` twice and asserts the returned seqno is stable; with the fix, the second call still sees `_new_cond` set (nothing composed yet) and recomputes the same value, so that invariant holds. The "conditions unchanged" case returns `cond_seqno` exactly when `new` is None and nothing is in flight for that table, matching the expected outputs in tests/ovsdb-idl.at. - I grepped for other uses of `condition_state.request`: lines 398 and 668 are proper method calls; no other truthiness-on-bound-method instances remain. Hunk under review: > if table.condition_state.new: > - any_reqs = any(t.condition_state.request > + any_reqs = any(t.condition_state.requested is not None > for t in self.tables.values()) > return self.cond_seqno + int(any_reqs) + 1 The change is correct. Two minor, non-functional observations: 1. Line 710 tests `if table.condition_state.new:` (truthiness) while line 717 uses `bool(table.condition_state.requested)` and the new line uses `is not None`. All three work here because a condition is always a non-empty list (`cond_change()` maps `[]` to `[False]` at lines 700-701, and `_ack_cond`/`_req_cond`/`_new_cond` are only ever set to lists or `None`), but the mixed idioms are mildly inconsistent. No behavioral impact. 2. Pre-existing, not worsened by this commit: if a reconnect happens while a condition is in flight, `sync_conditions()` (lines 364-408) can clear `requested` via `ack()` without incrementing `cond_seqno`, so a seqno previously returned by `cond_change()` may never be reached. That is an artifact of the FSM restart path and outside this change's scope. Verdict: PASS _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
