Terry Wilson via dev <[email protected]> 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 <[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: 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
