On Wed, Feb 28, 2024 at 8:41 AM Ilya Maximets <i.maxim...@ovn.org> wrote: > > On 2/22/24 17:37, Mike Pattrick wrote: > > Previously when an empty mutation was used to count the number of rows > > in a table, OVSDB would iterate over all rows twice. First to perform an > > RBAC check, and then to perform the no-operation. > > > > This change adds a short circuit to mutate operations with no conditions > > and an empty mutation set, returning immediately. One notable change in > > functionality is not performing the RBAC check in this condition, as no > > mutation actually takes place. > > > > Reported-by: Terry Wilson <twil...@redhat.com> > > Reported-at: https://issues.redhat.com/browse/FDP-359 > > Signed-off-by: Mike Pattrick <m...@redhat.com> > > --- > > v2: Added additional non-rbac tests, and support for conditional > > counting without the rbac check > > --- > > ovsdb/execution.c | 26 +++++++++++++++++++- > > ovsdb/mutation.h | 6 +++++ > > tests/ovsdb-execution.at | 51 ++++++++++++++++++++++++++++++++++++++++ > > tests/ovsdb-rbac.at | 23 ++++++++++++++++++ > > 4 files changed, 105 insertions(+), 1 deletion(-) > > Hi, Mike. Thanks for v2! I didn't test, but it looks good in general. > See one comment inline. > > Best regards, Ilya Maximets. > > > > > diff --git a/ovsdb/execution.c b/ovsdb/execution.c > > index 8c20c3b54..7ed700632 100644 > > --- a/ovsdb/execution.c > > +++ b/ovsdb/execution.c > > @@ -585,6 +585,19 @@ mutate_row_cb(const struct ovsdb_row *row, void *mr_) > > return *mr->error == NULL; > > } > > > > +struct count_row_cbdata { > > + size_t n_matches; > > +}; > > Do we actually need this structure? It only has one element. > We should be able to just pass a counter around directly.
It seemed more thematic to me at the time, but I can change this. -M > > > + > > +static bool > > +count_row_cb(const struct ovsdb_row *row OVS_UNUSED, void *cr_) > > +{ > > + struct count_row_cbdata *cr = cr_; > > + > > + cr->n_matches++; > > + return true; > > +} > > + > > static struct ovsdb_error * > > ovsdb_execute_mutate(struct ovsdb_execution *x, struct ovsdb_parser > > *parser, > > struct json *result) > > @@ -609,7 +622,18 @@ ovsdb_execute_mutate(struct ovsdb_execution *x, struct > > ovsdb_parser *parser, > > error = ovsdb_condition_from_json(table->schema, where, x->symtab, > > &condition); > > } > > - if (!error) { > > + if (!error && ovsdb_mutation_set_empty(&mutations)) { > > + /* Special case with no mutations, just return the row count. */ > > + if (ovsdb_condition_empty(&condition)) { > > + json_object_put(result, "count", > > + json_integer_create(hmap_count(&table->rows))); > > + } else { > > + struct count_row_cbdata cr = {}; > > + ovsdb_query(table, &condition, count_row_cb, &cr); > > + json_object_put(result, "count", > > + json_integer_create(cr.n_matches)); > > + } > > + } else if (!error) { > > mr.n_matches = 0; > > mr.txn = x->txn; > > mr.mutations = &mutations; > > diff --git a/ovsdb/mutation.h b/ovsdb/mutation.h > > index 7566ef199..05d4a262a 100644 > > --- a/ovsdb/mutation.h > > +++ b/ovsdb/mutation.h > > @@ -69,4 +69,10 @@ void ovsdb_mutation_set_destroy(struct > > ovsdb_mutation_set *); > > struct ovsdb_error *ovsdb_mutation_set_execute( > > struct ovsdb_row *, const struct ovsdb_mutation_set *) > > OVS_WARN_UNUSED_RESULT; > > > > +static inline bool ovsdb_mutation_set_empty( > > + const struct ovsdb_mutation_set *ms) > > +{ > > + return ms->n_mutations == 0; > > +} > > + > > #endif /* ovsdb/mutation.h */ > > diff --git a/tests/ovsdb-execution.at b/tests/ovsdb-execution.at > > index fd1c7a239..1ffa2b738 100644 > > --- a/tests/ovsdb-execution.at > > +++ b/tests/ovsdb-execution.at > > @@ -1201,4 +1201,55 @@ OVSDB_CHECK_EXECUTION([garbage collection], > > [{"rows":[]}] > > ]])]) > > > > +OVSDB_CHECK_EXECUTION([insert rows, count with mutation], > > + [ordinal_schema], > > + [[[["ordinals", > > + {"op": "insert", > > + "table": "ordinals", > > + "row": {"number": 0, "name": "zero"}, > > + "uuid-name": "first"}]]], > > + [[["ordinals", > > + {"op": "insert", > > + "table": "ordinals", > > + "row": {"number": 1, "name": "one"}, > > + "uuid-name": "first"}]]], > > + [[["ordinals", > > + {"op": "mutate", > > + "table": "ordinals", > > + "where": [["name", "==", "zero"]], > > + "mutations": []}]]], > > + [[["ordinals", > > + {"op": "mutate", > > + "table": "ordinals", > > + "where": [["name", "==", "one"]], > > + "mutations": []}]]], > > + [[["ordinals", > > + {"op": "insert", > > + "table": "ordinals", > > + "row": {"number": 2, "name": "one"}, > > + "uuid-name": "first"}]]], > > + [[["ordinals", > > + {"op": "mutate", > > + "table": "ordinals", > > + "where": [["name", "==", "one"]], > > + "mutations": []}]]], > > + [[["ordinals", > > + {"op": "delete", > > + "table": "ordinals", > > + "where": [["name", "==", "zero"]]}]]], > > + [[["ordinals", > > + {"op": "mutate", > > + "table": "ordinals", > > + "where": [], > > + "mutations": []}]]]], > > + [[[{"uuid":["uuid","<0>"]}] > > +[{"uuid":["uuid","<1>"]}] > > +[{"count":1}] > > +[{"count":1}] > > +[{"uuid":["uuid","<2>"]}] > > +[{"count":2}] > > +[{"count":1}] > > +[{"count":2}] > > +]]) > > + > > EXECUTION_EXAMPLES > > diff --git a/tests/ovsdb-rbac.at b/tests/ovsdb-rbac.at > > index 3172e4bf5..c1e5a9134 100644 > > --- a/tests/ovsdb-rbac.at > > +++ b/tests/ovsdb-rbac.at > > @@ -355,6 +355,29 @@ AT_CHECK([uuidfilt stdout], [0], [[[{"details":"RBAC > > rules for client \"client-2 > > ], [ignore]) > > > > # Test 14: > > +# Count the rows in other_colors. This should pass even though the RBAC > > +# authorization would fail because "client-2" does not match the > > +# "creator" column for this row. Because the RBAC check is bypassed when > > +# mutation is empty. > > +AT_CHECK([ovsdb-client transact ssl:127.0.0.1:$SSL_PORT \ > > + --private-key=$RBAC_PKIDIR/client-2-privkey.pem \ > > + --certificate=$RBAC_PKIDIR/client-2-cert.pem \ > > + --ca-cert=$RBAC_PKIDIR/pki/switchca/cacert.pem \ > > + ['["mydb", > > + {"op": "mutate", > > + "table": "other_colors", > > + "where": [], > > + "mutations": []}, > > + {"op": "mutate", > > + "table": "other_colors", > > + "where": [["name", "==", "seafoam"]], > > + "mutations": []} > > + ]']], [0], [stdout], [ignore]) > > +cat stdout >> output > > +AT_CHECK([uuidfilt stdout], [0], [[[{"count":1},{"count":1}]] > > +], [ignore]) > > + > > +# Test 15: > > # Attempt to delete a row from the "other_colors" table. This should pass > > # the RBAC authorization test because "client-1" does matches the > > # "creator" column for this row. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev