[PWCI] "[ovs-dev,RFC] ovsdb-idl: Add a way to set IDL txn as read only."#7
Open
[PWCI] "[ovs-dev,RFC] ovsdb-idl: Add a way to set IDL txn as read only."#7
Conversation
For vport devices hardware offload counters were being incremented on the physical device but decremented on the vport, causing incorrect statistics that didn't return to zero when flows are removed. Fix by using physdev instead of netdev for counter decrements to match the increment behavior. Fixes: 0e6366c ("netdev-offload-dpdk: Implement hw-offload statistics read.") Signed-off-by: Linda Wang <linda.wang@jaguarmicro.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
The error paths should release any allocated resources that are not added to the patterns. Found by Coverity. Fixes: c1a5d0e ("netdev-offload-dpdk: Support IPv4 fragmentation types.") Fixes: 1318929 ("netdev-offload-dpdk: Support IPv6 fragmentation types.") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> Reviewed-by: David Marchand <david.marchand@redhat.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Coverity reported an issue where the oftable->vacancy_up/down members were read without holding the ofproto_mutex. This patch ensures the mutex is acquired before accessing these members and adds OVS_REQUIRES(ofproto_mutex) to the affected function to enforce proper locking. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Fixes: bab8601 ("Implement Vacancy Events for OFPMP_TABLE_DESC.")
This patch resolves a Coverity-reported issue where a dereference could occur after a null check in ovsdb_monitor_row_update_type(). An additional condition has been added to ensure a safe early exit in ovsdb_monitor_compose_row_update(). Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity reported a possible null dereference of t->reply when handling errors in the "committing" state for "transact" requests. In practice, t->reply is always non-NULL at this point, but static analysis cannot infer this guarantee. Adding ovs_assert(t->reply) documents the invariant for developers and ensures debug builds will catch any misuse. No change in runtime behavior; this only improves code safety and satisfies static analysis. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity reported a possible null dereference of ctx->db inside the event processing loop in ovsdb_relay_run(). In practice, ctx->db is guaranteed to be non-NULL for valid relay_ctx structures. Adding ovs_assert(ctx->db) documents this invariant and ensures debug builds catch any misuse. No change in runtime behavior; this only improves code safety and satisfies static analysis. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity flagged a potential null dereference of txnid when calling raft_next_entry() in ovsdb_storage_read(). To prevent this, raft_next_entry() now checks whether txnid is non-NULL before attempting to write to it. This preserves existing behavior when txnid is valid, while safely handling the case where it is NULL. No change to normal operation; this only prevents potential crashes if a NULL txnid is passed. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity reported a possible null dereference of datum->values when handling value-type weak references in find_and_add_weak_refs(). In practice, datum->values must be non-NULL for value-type weak references. Adding ovs_assert(datum->values) documents this invariant and ensures that we catch any unexpected misuse. No change to runtime behavior in production builds; this only improves safety and satisfies static analysis. Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
We're not installing anything from FreeBSD-kmods or any other non-main
repository, but it seems to have mismatching packages with the latest
Google Cloud image that appears to not be updated yet:
# pkg update -f
Updating FreeBSD repository catalogue...
Fetching meta.conf: . done
Fetching data.pkg: .......... done
Processing entries: .......... done
FreeBSD repository update completed. 36502 packages processed.
Updating FreeBSD-kmods repository catalogue...
Fetching meta.conf: . done
Fetching data.pkg: .. done
Processing entries:
Newer FreeBSD version for package parallels-tools:
To ignore this error set IGNORE_OSVERSION=yes
- package: 1403506
- running userland: 1403505
Ignore the mismatch and continue? [y/N]:
pkg: repository FreeBSD-kmods contains packages for wrong OS version:
FreeBSD:14:amd64
Processing entries...
Unable to update repository FreeBSD-kmods
Error updating repositories!
Not updating FreeBSD-kmods should avoid this issue. In the worst case
the next 'pkg install' will fail, but it shouldn't really ever happen
as we do not install any kernel modules or packages that depend on
any kernel modules.
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
When adjusting bond parameters, any adjustment is considered sufficient for triggering a rebalance. This is a very simplistic config update scheme that triggers a complete rebalance even if the time adjustment would move the next expiration out beyond the last calculated expiration. For the interval parameter only, we can simply recalculate the expiry deadline and let the next bond_run() event do the rebalance if needed. Even if the recalculation would cause the deadline to have occurred in the past, it should execute on the next bond_run() anyway. This is still okay, as the rebalance interval timeout may not result in a full rebalance anyway. Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2025-April/053588.html Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Aaron Conole <aconole@redhat.com>
Add a way to set IDL txn as read only, that is useful in cases when we want to ensure that the program doesn't write anything into the txn. This is done via assert because this is considered a bug which should be fixed in the IDL caller. Signed-off-by: Ales Musil <amusil@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideThis PR introduces read-only support for ovsdb-idl transactions by adding a new flag, preventing writes when set, and providing a setter and test updates, along with a series of targeted fixes and enhancements across bond rebalancing, DPDK offload parsing, ofproto table queries, Raft entry handling, CI configuration, monitor update logic, relay context validation, and assertion strengthening in transaction and trigger modules. Sequence diagram for write operation with read-only transaction in ovsdb-idlsequenceDiagram
participant App
participant IDL_Txn
App->>IDL_Txn: Attempt write operation
alt txn.read_only == true
IDL_Txn-->>App: Assert fails, write blocked
else txn.read_only == false
IDL_Txn-->>App: Write proceeds
end
Class diagram for updated ovsdb_idl_txn with read-only supportclassDiagram
class ovsdb_idl_txn {
enum ovsdb_idl_txn_status status
char *error
bool dry_run
bool read_only
struct ds comment
+void ovsdb_idl_txn_set_read_only(bool read_only)
}
ovsdb_idl_txn : ovsdb_idl_txn_set_read_only()
Class diagram for bond reconfiguration logic updateclassDiagram
class bond {
long long int next_rebalance
int rebalance_interval
int balance
+void bond_reconfigure(const bond_settings *s)
}
class bond_settings {
int rebalance_interval
int balance
}
bond --> bond_settings : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughIntroduces IDL transaction read-only mode with API, assertions, and tests; adjusts test tool to pass read-only flag. Fixes a DPDK memory leak and corrects offload counter source. Tightens locking in OpenFlow table-desc paths. Tweaks bond rebalance timing logic. Refines several OVSDB internals and CI config. Updates AUTHORS. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as test-ovsdb CLI
participant Tool as test-ovsdb
participant IDL as ovsdb_idl
participant TXN as ovsdb_idl_txn
participant Server as ovsdb-server
CLI->>Tool: parse args (--read-only)
Tool->>IDL: connect / setup
Tool->>TXN: create transaction
Tool->>TXN: ovsdb_idl_txn_set_read_only(true)
Note over TXN: txn.read_only = true
Tool->>TXN: attempt write (insert/set/delete)
alt read-only (debug)
TXN-->>Tool: assert(!read_only) fails
Tool-->>CLI: report assertion failure
else not read-only
TXN->>Server: send write ops
Server-->>Tool: txn result
end
sequenceDiagram
autonumber
participant Cfg as bond_reconfigure
participant Bond as bond state
Cfg->>Bond: read old/new rebalance_interval
alt both non-zero
Cfg->>Bond: compute old_start = next - old_interval
Cfg->>Bond: next = old_start + new_interval
Cfg->>Bond: update interval (no revalidate)
else toggle to/from zero
Cfg->>Bond: set revalidate = true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- This PR bundles unrelated changes across IDL read-only txn, ofproto rebalance logic, DPDK parsing, raft, CI, and more; please split into focused, domain-specific patches to simplify review.
- The new read_only txn flag currently relies on ovs_assert to block writes—consider returning a proper error instead of crashing in production builds.
- In parse_flow_match error paths, only spec/mask are freed on some branches—ensure all allocated resources are consistently freed to avoid leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This PR bundles unrelated changes across IDL read-only txn, ofproto rebalance logic, DPDK parsing, raft, CI, and more; please split into focused, domain-specific patches to simplify review.
- The new read_only txn flag currently relies on ovs_assert to block writes—consider returning a proper error instead of crashing in production builds.
- In parse_flow_match error paths, only spec/mask are freed on some branches—ensure all allocated resources are consistently freed to avoid leaks.
## Individual Comments
### Comment 1
<location> `lib/ovsdb-idl.c:2844` </location>
<code_context>
ovs_assert(!txn->inc_table);
ovs_assert(column->type.key.type == OVSDB_TYPE_INTEGER);
ovs_assert(column->type.value.type == OVSDB_TYPE_VOID);
+ ovs_assert(!txn->read_only);
txn->inc_table = row->table->class_->name;
</code_context>
<issue_to_address>
Using ovs_assert for read_only enforcement may not be sufficient for production.
Assertions are disabled in release builds, so this check won't prevent writes on read-only transactions in production. Please implement a runtime error or graceful failure instead.
Suggested implementation:
```c
if (txn->read_only) {
txn->error = xstrdup("Attempted write on read-only transaction");
VLOG_ERR("%s", txn->error);
return; // Or handle error as appropriate for this function's return type
}
```
- Ensure that the function containing this code supports early return or error handling. If it does not, you may need to refactor the function to allow for graceful failure.
- If the function has a specific return type (e.g., int, bool), replace `return;` with an appropriate error return value.
- Make sure `xstrdup` and `VLOG_ERR` are available and included in the file.
- If there is a standard error handling mechanism in your codebase, use that instead of setting `txn->error` directly.
</issue_to_address>
### Comment 2
<location> `ofproto/bond.c:462` </location>
<code_context>
}
if (bond->rebalance_interval != s->rebalance_interval) {
+ if (s->rebalance_interval && bond->rebalance_interval) {
+ /* Recompute the next rebalance interval by moving the
+ * next_rebalance to be offset by the new interval. In this
+ * case, if all that was updated is the rebalance interval,
+ * we can skip triggering the rest of the port reconfiguration. */
+ if (bond->next_rebalance) {
+ long long int old_start_time =
+ bond->next_rebalance - s->rebalance_interval;
+ bond->next_rebalance =
+ old_start_time + bond->rebalance_interval;
+ }
+ } else {
+ /* When the bond is doing a disable/enable of the rebalance
</code_context>
<issue_to_address>
The logic for updating next_rebalance may not handle all edge cases.
Please verify that skipping the adjustment when next_rebalance is zero or unset is intentional, particularly for initial setups or after extended inactivity.
</issue_to_address>
### Comment 3
<location> `ovsdb/raft.c:3625` </location>
<code_context>
}
raft->last_applied++;
- *eid = e->eid;
+ if (eid) {
+ *eid = e->eid;
+ }
</code_context>
<issue_to_address>
The new null check for eid may mask programming errors.
Log or handle unexpected NULL values for eid to help identify underlying logic errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (eid) {
*eid = e->eid;
}
=======
if (eid) {
*eid = e->eid;
} else {
VLOG_ERR("Unexpected NULL eid pointer in raft entry application");
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `tests/test-ovsdb.c:61` </location>
<code_context>
enum ovsdb_idl_txn_status status;
char *error;
bool dry_run;
+ bool read_only;
struct ds comment;
</code_context>
<issue_to_address>
Missing explicit unit tests for read-only transaction enforcement.
Please add tests that perform write operations in read-only mode and confirm they fail as expected.
Suggested implementation:
```c
struct test_ovsdb_pvt_context {
bool write_changed_only;
bool track;
bool read_only;
};
/* Test: Write operations in read-only transaction should fail */
static void
test_read_only_transaction_enforcement(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("dummy.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
/* Set transaction to read-only */
txn->read_only = true;
/* Try to insert a row */
struct ovsdb_idl_row *row = ovsdb_idl_txn_insert(txn, idl->tables[0]);
assert(row == NULL);
/* Try to update a row */
struct ovsdb_idl_row *existing_row = ovsdb_idl_first_row(idl, idl->tables[0]);
if (existing_row) {
bool update_result = ovsdb_idl_txn_write(txn, existing_row, idl->tables[0]->columns[0], ovsdb_datum_create_string("new_value"));
assert(!update_result);
}
/* Try to delete a row */
if (existing_row) {
bool delete_result = ovsdb_idl_txn_delete(txn, existing_row);
assert(!delete_result);
}
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}
```
```c
/* Magic to pass to ovsdb_log_open(). */
{"verbose", optional_argument, NULL, 'v'},
{"change-track", optional_argument, NULL, 'c'},
{"write-changed-only", optional_argument, NULL, 'w'},
{"read-only", optional_argument, NULL, 'r'},
{"magic", required_argument, NULL, OPT_MAGIC},
{"no-rename-open-files", no_argument, NULL, OPT_NO_RENAME_OPEN_FILES},
{"help", no_argument, NULL, 'h'},
pvt->write_changed_only = true;
break;
/* Register the new test */
int
main(int argc, char *argv[])
{
// ... existing test registration code ...
test_read_only_transaction_enforcement();
// ... rest of main ...
}
```
- You may need to adjust the table/column references (`idl->tables[0]`, `idl->tables[0]->columns[0]`) to match your actual schema.
- Ensure that the helper functions (`ovsdb_idl_txn_insert`, `ovsdb_idl_txn_write`, `ovsdb_idl_txn_delete`, etc.) return NULL or false on failure as expected.
- If your test framework uses a different test registration mechanism, integrate `test_read_only_transaction_enforcement()` accordingly.
</issue_to_address>
### Comment 5
<location> `tests/test-ovsdb.c:2481` </location>
<code_context>
txn = ovsdb_idl_txn_create(idl);
ovsdb_idl_check_consistency(idl);
+ ovsdb_idl_txn_set_read_only(txn, read_only);
for (cmd = strtok_r(commands, ",", &save_ptr1); cmd;
cmd = strtok_r(NULL, ",", &save_ptr1)) {
</code_context>
<issue_to_address>
No negative test cases for write operations in read-only mode.
Add tests that attempt writes in read-only mode and confirm the system returns an error, ensuring enforcement cannot be bypassed.
Suggested implementation:
```c
/* Existing tests and includes... */
/* Negative test: Attempt to insert in read-only mode */
static void
test_insert_in_read_only_mode(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("test.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
ovsdb_idl_txn_set_read_only(txn, true);
/* Attempt to insert a row */
struct ovsdb_idl_row *row = ovsdb_idl_txn_insert(txn, ovsdb_idl_table_from_name(idl, "TestTable"));
ovsdb_idl_txn_commit(txn);
enum ovsdb_idl_txn_status status = ovsdb_idl_txn_get_status(txn);
assert(status == TXN_ERROR || status == TXN_UNCHANGED);
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}
/* Negative test: Attempt to update in read-only mode */
static void
test_update_in_read_only_mode(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("test.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
ovsdb_idl_txn_set_read_only(txn, true);
/* Attempt to update a row (simulate by setting a column value) */
struct ovsdb_idl_row *row = ovsdb_idl_first_row(idl, ovsdb_idl_table_from_name(idl, "TestTable"));
if (row) {
ovsdb_idl_txn_write(row, ovsdb_idl_column_from_name(idl, "TestTable", "name"), "new_value");
ovsdb_idl_txn_commit(txn);
enum ovsdb_idl_txn_status status = ovsdb_idl_txn_get_status(txn);
assert(status == TXN_ERROR || status == TXN_UNCHANGED);
}
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}
/* Negative test: Attempt to delete in read-only mode */
static void
test_delete_in_read_only_mode(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("test.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
ovsdb_idl_txn_set_read_only(txn, true);
/* Attempt to delete a row */
struct ovsdb_idl_row *row = ovsdb_idl_first_row(idl, ovsdb_idl_table_from_name(idl, "TestTable"));
if (row) {
ovsdb_idl_txn_delete(row);
ovsdb_idl_txn_commit(txn);
enum ovsdb_idl_txn_status status = ovsdb_idl_txn_get_status(txn);
assert(status == TXN_ERROR || status == TXN_UNCHANGED);
}
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}
```
```c
int
main(int argc, char *argv[])
{
/* existing test runner code */
/* Run negative tests for read-only enforcement */
test_insert_in_read_only_mode();
test_update_in_read_only_mode();
test_delete_in_read_only_mode();
/* ...rest of main... */
```
- You may need to adjust table/column names ("TestTable", "name") to match your schema.
- Ensure that the error status constants (`TXN_ERROR`, `TXN_UNCHANGED`) match those defined in your codebase.
- If your test runner uses a different mechanism for running tests, integrate these functions accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ovs_assert(!txn->inc_table); | ||
| ovs_assert(column->type.key.type == OVSDB_TYPE_INTEGER); | ||
| ovs_assert(column->type.value.type == OVSDB_TYPE_VOID); | ||
| ovs_assert(!txn->read_only); |
There was a problem hiding this comment.
suggestion (bug_risk): Using ovs_assert for read_only enforcement may not be sufficient for production.
Assertions are disabled in release builds, so this check won't prevent writes on read-only transactions in production. Please implement a runtime error or graceful failure instead.
Suggested implementation:
if (txn->read_only) {
txn->error = xstrdup("Attempted write on read-only transaction");
VLOG_ERR("%s", txn->error);
return; // Or handle error as appropriate for this function's return type
}- Ensure that the function containing this code supports early return or error handling. If it does not, you may need to refactor the function to allow for graceful failure.
- If the function has a specific return type (e.g., int, bool), replace
return;with an appropriate error return value. - Make sure
xstrdupandVLOG_ERRare available and included in the file. - If there is a standard error handling mechanism in your codebase, use that instead of setting
txn->errordirectly.
| @@ -459,8 +459,23 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s) | |||
| } | |||
There was a problem hiding this comment.
question: The logic for updating next_rebalance may not handle all edge cases.
Please verify that skipping the adjustment when next_rebalance is zero or unset is intentional, particularly for initial setups or after extended inactivity.
Comment on lines
-3625
to
3627
| raft->last_applied++; | ||
| *eid = e->eid; | ||
| if (eid) { | ||
| *eid = e->eid; | ||
| } | ||
|
|
||
| /* DB will only read each entry once, so we don't need to store the fully |
There was a problem hiding this comment.
suggestion (bug_risk): The new null check for eid may mask programming errors.
Log or handle unexpected NULL values for eid to help identify underlying logic errors.
Suggested change
| *eid = e->eid; | |
| if (eid) { | |
| *eid = e->eid; | |
| } | |
| /* DB will only read each entry once, so we don't need to store the fully | |
| if (eid) { | |
| *eid = e->eid; | |
| } else { | |
| VLOG_ERR("Unexpected NULL eid pointer in raft entry application"); | |
| } |
| struct test_ovsdb_pvt_context { | ||
| bool write_changed_only; | ||
| bool track; | ||
| bool read_only; |
There was a problem hiding this comment.
suggestion (testing): Missing explicit unit tests for read-only transaction enforcement.
Please add tests that perform write operations in read-only mode and confirm they fail as expected.
Suggested implementation:
struct test_ovsdb_pvt_context {
bool write_changed_only;
bool track;
bool read_only;
};
/* Test: Write operations in read-only transaction should fail */
static void
test_read_only_transaction_enforcement(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("dummy.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
/* Set transaction to read-only */
txn->read_only = true;
/* Try to insert a row */
struct ovsdb_idl_row *row = ovsdb_idl_txn_insert(txn, idl->tables[0]);
assert(row == NULL);
/* Try to update a row */
struct ovsdb_idl_row *existing_row = ovsdb_idl_first_row(idl, idl->tables[0]);
if (existing_row) {
bool update_result = ovsdb_idl_txn_write(txn, existing_row, idl->tables[0]->columns[0], ovsdb_datum_create_string("new_value"));
assert(!update_result);
}
/* Try to delete a row */
if (existing_row) {
bool delete_result = ovsdb_idl_txn_delete(txn, existing_row);
assert(!delete_result);
}
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}/* Magic to pass to ovsdb_log_open(). */
{"verbose", optional_argument, NULL, 'v'},
{"change-track", optional_argument, NULL, 'c'},
{"write-changed-only", optional_argument, NULL, 'w'},
{"read-only", optional_argument, NULL, 'r'},
{"magic", required_argument, NULL, OPT_MAGIC},
{"no-rename-open-files", no_argument, NULL, OPT_NO_RENAME_OPEN_FILES},
{"help", no_argument, NULL, 'h'},
pvt->write_changed_only = true;
break;
/* Register the new test */
int
main(int argc, char *argv[])
{
// ... existing test registration code ...
test_read_only_transaction_enforcement();
// ... rest of main ...
}- You may need to adjust the table/column references (
idl->tables[0],idl->tables[0]->columns[0]) to match your actual schema. - Ensure that the helper functions (
ovsdb_idl_txn_insert,ovsdb_idl_txn_write,ovsdb_idl_txn_delete, etc.) return NULL or false on failure as expected. - If your test framework uses a different test registration mechanism, integrate
test_read_only_transaction_enforcement()accordingly.
|
|
||
| txn = ovsdb_idl_txn_create(idl); | ||
| ovsdb_idl_check_consistency(idl); | ||
| ovsdb_idl_txn_set_read_only(txn, read_only); |
There was a problem hiding this comment.
suggestion (testing): No negative test cases for write operations in read-only mode.
Add tests that attempt writes in read-only mode and confirm the system returns an error, ensuring enforcement cannot be bypassed.
Suggested implementation:
/* Existing tests and includes... */
/* Negative test: Attempt to insert in read-only mode */
static void
test_insert_in_read_only_mode(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("test.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
ovsdb_idl_txn_set_read_only(txn, true);
/* Attempt to insert a row */
struct ovsdb_idl_row *row = ovsdb_idl_txn_insert(txn, ovsdb_idl_table_from_name(idl, "TestTable"));
ovsdb_idl_txn_commit(txn);
enum ovsdb_idl_txn_status status = ovsdb_idl_txn_get_status(txn);
assert(status == TXN_ERROR || status == TXN_UNCHANGED);
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}
/* Negative test: Attempt to update in read-only mode */
static void
test_update_in_read_only_mode(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("test.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
ovsdb_idl_txn_set_read_only(txn, true);
/* Attempt to update a row (simulate by setting a column value) */
struct ovsdb_idl_row *row = ovsdb_idl_first_row(idl, ovsdb_idl_table_from_name(idl, "TestTable"));
if (row) {
ovsdb_idl_txn_write(row, ovsdb_idl_column_from_name(idl, "TestTable", "name"), "new_value");
ovsdb_idl_txn_commit(txn);
enum ovsdb_idl_txn_status status = ovsdb_idl_txn_get_status(txn);
assert(status == TXN_ERROR || status == TXN_UNCHANGED);
}
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}
/* Negative test: Attempt to delete in read-only mode */
static void
test_delete_in_read_only_mode(void)
{
struct ovsdb_idl *idl = ovsdb_idl_create("test.db", NULL, false, false);
struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
ovsdb_idl_txn_set_read_only(txn, true);
/* Attempt to delete a row */
struct ovsdb_idl_row *row = ovsdb_idl_first_row(idl, ovsdb_idl_table_from_name(idl, "TestTable"));
if (row) {
ovsdb_idl_txn_delete(row);
ovsdb_idl_txn_commit(txn);
enum ovsdb_idl_txn_status status = ovsdb_idl_txn_get_status(txn);
assert(status == TXN_ERROR || status == TXN_UNCHANGED);
}
ovsdb_idl_txn_destroy(txn);
ovsdb_idl_destroy(idl);
}int
main(int argc, char *argv[])
{
/* existing test runner code */
/* Run negative tests for read-only enforcement */
test_insert_in_read_only_mode();
test_update_in_read_only_mode();
test_delete_in_read_only_mode();
/* ...rest of main... */- You may need to adjust table/column names ("TestTable", "name") to match your schema.
- Ensure that the error status constants (
TXN_ERROR,TXN_UNCHANGED) match those defined in your codebase. - If your test runner uses a different mechanism for running tests, integrate these functions accordingly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ovsdb/relay.c (1)
345-358: Potential use of uninitialized pointer ‘du’ on error path.If ovsdb_cs_parse_db_update() returns an error without setting 'du', the unconditional destroy risks UB.
Apply this diff:
- struct ovsdb_cs_db_update *du; + struct ovsdb_cs_db_update *du = NULL; @@ - ovsdb_cs_db_update_destroy(du); + if (du) { + ovsdb_cs_db_update_destroy(du); + }tests/ovsdb-idl.at (1)
3005-3046: Tests rely on assertion text; make them robust in non-assert buildsThe grep against “assertion !.*txn->read_only failed” ties correctness to assert-enabled binaries and exact message wording. In release configs (or differing assert formatting), these will silently pass/fail.
Consider one of:
- Make write paths return a deterministic error (e.g., TXN_ERROR or explicit exit code) and assert on that in tests.
- Gate these tests on assertions being enabled, or have test-ovsdb print a stable marker on read-only violation.
I can propose a minimal change in test-ovsdb to emit a fixed “IDL_READ_ONLY_VIOLATION” line on failure if you prefer.
♻️ Duplicate comments (6)
lib/ovsdb-idl.c (5)
3657-3657: Same assert-only enforcement issue as above.
ovs_assert(!row->table->idl->txn->read_only);won’t protect release builds. The commit-time guard proposed earlier will cover this path too.
3841-3842: Same assert-only enforcement issue as above (delete path).Covered by the commit-time guard change.
3871-3872: Same assert-only enforcement issue as above (insert path).Covered by the commit-time guard change.
4230-4232: Same assert-only enforcement issue as above (map mutate path).Covered by the commit-time guard change.
4268-4270: Same assert-only enforcement issue as above (set mutate path).Covered by the commit-time guard change.
tests/test-ovsdb.c (1)
136-139: Flag handling OK; verify default init as noted.Setting
pvt->read_only = true;on-ris fine. Ensure the default is false via themain()fix above.
🧹 Nitpick comments (8)
ofproto/bond.c (1)
461-479: Tiny readability tweak: name intervals explicitly.Consider introducing local variables old_interval/new_interval to avoid mix-ups like the one above.
- if (bond->rebalance_interval != s->rebalance_interval) { + if (bond->rebalance_interval != s->rebalance_interval) { + const int old_interval = bond->rebalance_interval; + const int new_interval = s->rebalance_interval; - if (s->rebalance_interval && bond->rebalance_interval) { + if (new_interval && old_interval) { ... - long long int old_start_time = - bond->next_rebalance - bond->rebalance_interval; /* old */ - bond->next_rebalance = - old_start_time + s->rebalance_interval; /* new */ + long long int old_start_time = bond->next_rebalance - old_interval; + bond->next_rebalance = old_start_time + new_interval; } else { revalidate = true; } - bond->rebalance_interval = s->rebalance_interval; + bond->rebalance_interval = new_interval; }ovsdb/relay.c (1)
383-386: Init the events list before passing it to ovsdb_cs_run().Ensure the list is initialized regardless of ovsdb_cs_run() internals.
Apply this diff:
struct ovs_list events; - ovs_assert(ctx->db && ctx->cs); + ovs_assert(ctx->db && ctx->cs); + ovs_list_init(&events); ovsdb_txn_forward_run(ctx->db, ctx->cs); ovsdb_cs_run(ctx->cs, &events);ovsdb/raft.c (1)
3611-3619: Update function comment to reflect eid can be NULL.Doc currently implies non-NULL. Clarify to match code.
Apply this diff:
-/* Returns the next log entry or snapshot from 'raft', or NULL if there are - * none left to read. Stores the entry ID of the log entry in '*eid'. +/* Returns the next log entry or snapshot from 'raft', or NULL if there are + * none left to read. If 'eid' is non-NULL, stores the entry ID of the log + * entry in '*eid'. * * The caller takes ownership of the result. */ovsdb/trigger.c (1)
401-401: Add defensive null check for t->reply in the “transact” path Wrap thejson_array_add(t->reply->result, …)andovsdb_trigger_complete()calls inif (t->reply) { json_array_add(t->reply->result, …); ovsdb_trigger_complete(t); } else { VLOG_ERR("missing reply in transact trigger"); return; }to prevent a null‐dereference in non-assert builds.
ofproto/ofproto.c (1)
4287-4300: Guarding query_tables_desc() with ofproto_mutex is good; add intent annotation and a tiny doc.
- Since this function acquires the lock internally, consider marking it OVS_EXCLUDED(ofproto_mutex) to document that callers must not hold the lock.
- n_tables is effectively immutable after init, but a brief comment helps future readers understand why the pre-allocation outside the lock is safe.
Apply:
static void -query_tables_desc(struct ofproto *ofproto, struct ofputil_table_desc **descp) +query_tables_desc(struct ofproto *ofproto, struct ofputil_table_desc **descp) + OVS_EXCLUDED(ofproto_mutex) { struct ofputil_table_desc *table_desc; size_t i; - table_desc = *descp = xcalloc(ofproto->n_tables, sizeof *table_desc); + /* n_tables is set once during init and never changes; safe to read unlocked. */ + table_desc = *descp = xcalloc(ofproto->n_tables, sizeof *table_desc); ovs_mutex_lock(&ofproto_mutex); for (i = 0; i < ofproto->n_tables; i++) { struct ofputil_table_desc *td = &table_desc[i]; query_table_desc__(td, ofproto, i); } ovs_mutex_unlock(&ofproto_mutex); }lib/ovsdb-idl.h (1)
387-390: Public API added; document behavior and guaranteesPlease add a short doc comment for ovsdb_idl_txn_set_read_only() clarifying:
- What operations are disallowed (all writes, inserts, deletes, partial updates).
- Runtime behavior in non-assert builds (return error vs. no-op vs. abort).
- Whether the flag can be toggled after writes have been staged.
This avoids ambiguity for downstreams and bindings.
lib/ovsdb-idl.c (1)
4580-4588: Setter semantics: consider guarding late toggles.If
read_onlyis enabled after writes were already queued, commit will now fail with the guard above, which is reasonable. Optionally, log a warning here when enabling read-only and the txn already has queued writes or increment to aid debugging.Example:
void ovsdb_idl_txn_set_read_only(struct ovsdb_idl_txn *txn, bool read_only) { if (txn) { txn->read_only = read_only; + if (read_only && + (!hmap_is_empty(&txn->txn_rows) || txn->inc_table != NULL)) { + VLOG_WARN("read-only set on a txn that already has write intents; " + "commit will fail with TXN_ERROR"); + } } }tests/test-ovsdb.c (1)
97-98: CLI option wire-up looks good.
--read-only/-ris added consistently with existing options.Also add this option to the help text for discoverability:
@@ static void usage(void) - " enable tracking of IDL changes\n"); + " enable tracking of IDL changes\n" + " -r, --read-only set IDL transactions to read-only\n");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
.cirrus.yml(1 hunks)AUTHORS.rst(1 hunks)lib/netdev-offload-dpdk.c(3 hunks)lib/ovsdb-idl.c(9 hunks)lib/ovsdb-idl.h(1 hunks)ofproto/bond.c(1 hunks)ofproto/ofproto.c(3 hunks)ovsdb/monitor.c(1 hunks)ovsdb/raft.c(1 hunks)ovsdb/relay.c(1 hunks)ovsdb/transaction.c(1 hunks)ovsdb/trigger.c(1 hunks)tests/ovsdb-idl.at(1 hunks)tests/test-ovsdb.c(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ofproto/bond.c (1)
ofproto/ofproto-dpif-upcall.c (1)
revalidate(2809-2989)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: linux gcc m32 --disable-ssl
- GitHub Check: linux clang --enable-afxdp
- GitHub Check: linux gcc check-ovsdb-cluster
- GitHub Check: linux gcc dpdk-shared --enable-shared
- GitHub Check: linux gcc --disable-ssl
- GitHub Check: linux gcc --enable-afxdp
- GitHub Check: linux gcc test -ljemalloc
- GitHub Check: linux gcc check check-dpdk dpdk
- GitHub Check: linux clang check check-dpdk dpdk
- GitHub Check: linux gcc test --enable-shared
- GitHub Check: linux clang c99
- GitHub Check: linux clang test -ljemalloc
- GitHub Check: linux clang --disable-ssl
- GitHub Check: linux clang test --enable-shared
- GitHub Check: linux gcc test
- GitHub Check: clang-analyze-cache
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-14-3-snap
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-13-5-snap
- GitHub Check: build oss-fuzz fuzzers
- GitHub Check: linux deb no dpdk
- GitHub Check: linux rpm fedora
- GitHub Check: osx clang --disable-ssl
🔇 Additional comments (14)
AUTHORS.rst (1)
298-299: LGTM: author entry added in correct alphabetical position.No further action.
.cirrus.yml (1)
17-21: Verify repository name ‘FreeBSD’ exists on these images.pkg update -f -r FreeBSD assumes a repo named “FreeBSD”. Please confirm both freebsd-13-5-snap and freebsd-14-3-snap images have that repo configured.
ovsdb/raft.c (1)
3625-3627: LGTM: NULL-guard for optional eid output avoids NPD.Safe, no behavior change otherwise.
ovsdb/transaction.c (1)
649-652: LGTM: assert documents required invariant for datum->values.Good defensive check for weak-ref value iteration.
ovsdb/monitor.c (1)
684-688: Correct: suppress “initial” update when new is absentReturning OJMS_NONE for initial rows without a ‘new’ value avoids spurious initial updates and aligns output with RFC 7047 expectations. Downstream handling (row_skip_update, compose paths) already treats OJMS_NONE correctly. LGTM.
ofproto/ofproto.c (2)
4331-4331: send_table_status now OVS_REQUIRES(ofproto_mutex) — good tightening.Callers (e.g., add_flow_finish(), delete_flows_finish__()) already run under the mutex, so this enforces correct usage and aligns with query_table_desc__’s contract.
4268-4268: Annotation correctly applied — verified that every call to query_table_desc__ and send_table_status occurs with ofproto_mutex held.lib/netdev-offload-dpdk.c (2)
1527-1533: Good catch: free IPv4 spec/mask on unknown frag pathThe added frees avoid leaking spec/mask on the error return.
1612-1617: Good catch: free IPv6 frag spec/mask on unknown frag pathThe added frees avoid leaking frag_spec/frag_mask on the error return.
lib/ovsdb-idl.c (2)
114-114: Field addition looks good.Adding
bool read_only;tostruct ovsdb_idl_txnis fine and well-scoped.
2775-2776: Proper default initialization.
txn->read_only = false;on creation is correct.tests/test-ovsdb.c (3)
2468-2469: Signature change is localized and sane.Adding
bool read_onlytoidl_set(...)is fine; it’s a file-local static and only used here.
2481-2482: Correct propagation of read-only to the txn.Calling
ovsdb_idl_txn_set_read_only(txn, read_only);early inidl_set()is correct.
2921-2922: Caller updated correctly.
idl_set(idl, arg, step++, pvt->read_only)passes the flag through as intended.
Comment on lines
2381
to
2384
| data = (struct netdev_offload_dpdk_data *) | ||
| ovsrcu_get(void *, &netdev->hw_info.offload_data); | ||
| ovsrcu_get(void *, &physdev->hw_info.offload_data); | ||
| data->rte_flow_counters[tid]--; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Decrement counter on physdev is correct; add NULL-guard
Flow creation increments counters using physdev; decrementing on physdev fixes accounting symmetry. However, ovsrcu_get() can return NULL (e.g., during teardown), so dereferencing data without a guard risks a crash.
Apply this diff to make it robust:
- data = (struct netdev_offload_dpdk_data *)
- ovsrcu_get(void *, &physdev->hw_info.offload_data);
- data->rte_flow_counters[tid]--;
+ data = (struct netdev_offload_dpdk_data *)
+ ovsrcu_get(void *, &physdev->hw_info.offload_data);
+ if (OVS_LIKELY(data)) {
+ data->rte_flow_counters[tid]--;
+ } else {
+ VLOG_DBG_RL(&rl, "%s/%s: offload_data is NULL while destroying "
+ "rte_flow 0x%"PRIxPTR, netdev_get_name(netdev),
+ netdev_get_name(physdev), (intptr_t) rte_flow);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| data = (struct netdev_offload_dpdk_data *) | |
| ovsrcu_get(void *, &netdev->hw_info.offload_data); | |
| ovsrcu_get(void *, &physdev->hw_info.offload_data); | |
| data->rte_flow_counters[tid]--; | |
| data = (struct netdev_offload_dpdk_data *) | |
| ovsrcu_get(void *, &physdev->hw_info.offload_data); | |
| if (OVS_LIKELY(data)) { | |
| data->rte_flow_counters[tid]--; | |
| } else { | |
| VLOG_DBG_RL(&rl, "%s/%s: offload_data is NULL while destroying " | |
| "rte_flow 0x%" PRIxPTR, | |
| netdev_get_name(netdev), | |
| netdev_get_name(physdev), | |
| (intptr_t) rte_flow); | |
| } |
🤖 Prompt for AI Agents
In lib/netdev-offload-dpdk.c around lines 2381 to 2384, ovsrcu_get() can return
NULL during teardown so dereferencing data is unsafe; add a NULL-guard: call
ovsrcu_get as shown, then check if (data) before decrementing
data->rte_flow_counters[tid] (if NULL, skip the decrement), keeping the current
reference semantics and avoiding a crash.
Comment on lines
+2844
to
2845
| ovs_assert(!txn->read_only); | ||
|
|
There was a problem hiding this comment.
Asserts aren’t sufficient to enforce read-only in production; add a runtime guard in commit().
Relying on ovs_assert(!txn->read_only) only protects debug builds. In release builds (with assertions compiled out), write ops will proceed. Enforce read-only at commit-time and fail fast if any writes (insert/update/delete/mutate/increment) are present.
Apply this diff to ovsdb_idl_txn_commit():
@@
enum ovsdb_idl_txn_status
ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
{
struct ovsdb_idl *idl = txn->idl;
if (txn != idl->txn) {
goto coverage_out;
} else if (!ovsdb_cs_may_send_transaction(idl->cs)) {
txn->status = TXN_TRY_AGAIN;
goto disassemble_out;
} else if (ovsdb_cs_get_lock(idl->cs) && !ovsdb_cs_has_lock(idl->cs)) {
txn->status = TXN_NOT_LOCKED;
goto disassemble_out;
}
+
+ /* Enforce read-only mode in all builds (not just with asserts). */
+ if (txn->read_only) {
+ bool has_writes = txn->inc_table != NULL;
+ struct ovsdb_idl_row *row;
+ HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) {
+ if (!row->new_datum /* delete */
+ || row->old_datum != row->new_datum /* insert/update */
+ || row->map_op_written || row->set_op_written) {
+ has_writes = true;
+ break;
+ }
+ }
+ if (has_writes) {
+ txn->status = TXN_ERROR;
+ free(txn->error);
+ txn->error = xstrdup("not allowed: read-only transaction attempted to modify data");
+ goto disassemble_out;
+ }
+ }
@@
struct json *operations = json_array_create_1(
json_string_create(idl->class_->database));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/ovsdb-idl.c around lines 2844-2845 (inside ovsdb_idl_txn_commit()),
replace the debug-only ovs_assert(!txn->read_only) with a runtime guard: if
txn->read_only and the transaction contains any recorded write operations (any
inserts/updates/deletes/mutates/increments or any change list/count indicating
writes), immediately fail the commit by setting the transaction error/status and
returning an error code to the caller (i.e., make the commit return failure
rather than proceeding), ensuring write attempts in read-only transactions are
rejected in release builds as well.
Comment on lines
+468
to
+472
| long long int old_start_time = | ||
| bond->next_rebalance - s->rebalance_interval; | ||
| bond->next_rebalance = | ||
| old_start_time + bond->rebalance_interval; | ||
| } |
There was a problem hiding this comment.
Fix incorrect interval used when recomputing next_rebalance.
You're subtracting the new interval and then adding the old one, which skews the schedule. Use old interval to derive the period start and new interval to set the next deadline.
Apply this diff:
- long long int old_start_time =
- bond->next_rebalance - s->rebalance_interval;
- bond->next_rebalance =
- old_start_time + bond->rebalance_interval;
+ long long int old_start_time =
+ bond->next_rebalance - bond->rebalance_interval; /* old */
+ bond->next_rebalance =
+ old_start_time + s->rebalance_interval; /* new */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| long long int old_start_time = | |
| bond->next_rebalance - s->rebalance_interval; | |
| bond->next_rebalance = | |
| old_start_time + bond->rebalance_interval; | |
| } | |
| long long int old_start_time = | |
| bond->next_rebalance - bond->rebalance_interval; /* old */ | |
| bond->next_rebalance = | |
| old_start_time + s->rebalance_interval; /* new */ | |
| } |
🤖 Prompt for AI Agents
In ofproto/bond.c around lines 468 to 472, the code incorrectly uses the new
interval when subtracting to compute the period start and then adds the old
interval back, skewing the schedule; change the subtraction to use the old
interval (bond->rebalance_interval) to compute old_start_time and then set
bond->next_rebalance = old_start_time + s->rebalance_interval (new interval) so
the next deadline is derived from the previous period start plus the updated
interval.
Comment on lines
+61
to
62
| bool read_only; | ||
| }; |
There was a problem hiding this comment.
New flag is fine, but default initialization elsewhere is missing.
Field addition is OK. However, pvt->read_only is not initialized in main(), leading to undefined behavior when -r is not provided.
Apply this diff in main() to zero-init the struct:
- struct test_ovsdb_pvt_context pvt = {.track = false};
+ struct test_ovsdb_pvt_context pvt = {0};
+ pvt.track = false;🤖 Prompt for AI Agents
In tests/test-ovsdb.c around lines 61-62, the newly added bool read_only field
is not initialized in main(), causing undefined behavior when the -r option is
not provided; update main() to zero-initialize the pvt struct (or explicitly set
pvt->read_only = false) when pvt is allocated/declared so the default is
deterministic, and ensure any existing initialization paths preserve or set this
flag consistently.
ovsrobot
pushed a commit
that referenced
this pull request
Sep 19, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
ovsrobot
pushed a commit
that referenced
this pull request
Oct 9, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Reproduce steps:
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs test -- set interface test type=internal
ip netns add ns_test
ip link set test netns ns_test
ip netns del ns_test
ifconfig br-ovs up
sleep 1
ifconfig br-ovs down
sleep 1
ovs-vsctl del-br br-ovs
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: This is an auto submission for "[ovs-dev,RFC] ovsdb-idl: Add a way to set IDL txn as read only.".
See "http://patchwork.ozlabs.org/project/openvswitch/list/?series=472384" for details.
Summary by Sourcery
Add support for read-only IDL transactions and integrate it into the test harness, optimize bond rebalance handling, introduce safety checks and correct several bugs across DPDK offload, ofproto concurrency, Raft entries, monitoring updates, and relay/transaction/trigger logic, and update the FreeBSD CI configuration.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests