| Message ID | 20260826043433.1832409-6-twilson@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python: Backport C fixes that never made it. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_FreeBSD_Build_and_Test | success | github build: passed |
Terry Wilson via dev <ovs-dev@openvswitch.org> writes: > Port of C commit d6db7b3cc06f ("ovsdb: add support for role-based access > controls"). A per-operation "not allowed" error is a hard error, and any > other unrecognized (non-"aborted") per-operation error is now recorded on > the transaction and logged as "transaction error" rather than being > silently ignored. > > Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> > Signed-off-by: Terry Wilson <twilson@redhat.com> > --- This is an AI generated review of your patch. A human has looked at the results and deemed any concerns as plausible. Commit summary: this change ports C commit d6db7b3cc06f ("ovsdb: add support for role-based access controls") to the Python IDL's transaction reply handling in `Transaction._process_reply` (python/ovs/db/idl.py). It adds an explicit `"not allowed"` per-operation error case (hard error, recorded on the transaction) and, for any other unrecognized non-`"aborted"` per-operation error, records the error JSON and emits a `vlog.warn("transaction error: ...")` instead of silently ignoring it. I compared against the C reference implementation in lib/ovsdb-idl.c (lines 4231-4268), verified `__set_error_json` (idl.py:2158-2160, only sets `self._error` if it is None), the `_error` initialization (idl.py:1797), and that `vlog.warn` exists in python/ovs/vlog.py:174. Hunk review: > elif error == "not allowed": > hard_errors = True > self.__set_error_json(op) > elif error != "aborted": > hard_errors = True > self.__set_error_json(op) > # XXX rate-limit > vlog.warn("transaction error: %s" % self._error) The control flow matches the C code at lib/ovsdb-idl.c:4247-4255: "not allowed" is a hard error with no log, "aborted" is ignored, anything else is a hard error plus a warning naming `txn->error` / `self._error`. Reading `self._error` immediately after `__set_error_json(op)` is safe: `_error` starts as `None` (idl.py:1797) and `ovs.json.to_string(op)` on the op dict always yields a non-None string, so the `%s` never formats a stale `None` in this path. Note that when multiple unrecognized errors occur in one reply, the warning shows the first recorded error rather than the current op's error (because `__set_error_json` only records the first) -- but this exactly mirrors the C behavior (`VLOG_WARN_RL(..., txn->error)`), so it is faithful to the port. Two minor observations, not regressions: 1. Fidelity gap for non-string "error" values. The C code checks `error->type == JSON_STRING` and reports a distinct message ("error in reply is not JSON string") when the field is not a string (lib/ovsdb-idl.c:4256-4261). In Python, a non-string error value simply fails all equality tests and falls into `elif error != "aborted"`, producing a hard error with the whole op dumped as "transaction error: ...". The status outcome (TXN_ERROR) is identical; only the log wording differs. Cosmetic. 2. The warning is not rate-limited, whereas C uses `VLOG_WARN_RL(&other_rl, ...)`. A misbehaving server returning unrecognized errors on every transaction could flood the log. The author acknowledges this with the `# XXX rate-limit` comment, which matches the existing convention throughout this file (e.g., idl.py:2251, 2287, 2292), and the Python vlog module has no rate-limiter to use. Pre-existing limitation of the Python library, not introduced here, but worth knowing it is a real behavioral difference from C rather than just a TODO. Also worth noting: functionally, `"not allowed"` was already treated as a hard error before this commit (it fell into the old `else` branch); the effective behavior change is only the added warning for unrecognized errors and the explicit named case for documentation/clarity. No callers depend on the old silent-ignore behavior (`_process_reply` is only reached via `__txn_process_reply`, idl.py:1290-1293). Verdict: PASS
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py index 07068407d..30bc6cb44 100644 --- a/python/ovs/db/idl.py +++ b/python/ovs/db/idl.py @@ -2269,11 +2269,14 @@ class Transaction(object): soft_errors = True elif error == "not owner": lock_errors = True - elif error == "aborted": - pass - else: + elif error == "not allowed": + hard_errors = True + self.__set_error_json(op) + elif error != "aborted": hard_errors = True self.__set_error_json(op) + # XXX rate-limit + vlog.warn("transaction error: %s" % self._error) else: hard_errors = True self.__set_error_json(op)
Port of C commit d6db7b3cc06f ("ovsdb: add support for role-based access controls"). A per-operation "not allowed" error is a hard error, and any other unrecognized (non-"aborted") per-operation error is now recorded on the transaction and logged as "transaction error" rather than being silently ignored. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Terry Wilson <twilson@redhat.com> --- python/ovs/db/idl.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)