ovsdb: Don't iterate over rows on empty mutation.

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 <[email protected]> Reported-at: https://issues.redhat.com/browse/FDP-359 Signed-off-by: Mike Pattrick <[email protected]> Signed-off-by: Ilya Maximets <[email protected]>
liwg06 · Mar 8, 2024 · 33f45de · 33f45de
1 parent 2c4ffd2
commit 33f45de
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 1 deletion.
diff --git a/ovsdb/execution.c b/ovsdb/execution.c
@@ -585,6 +585,16 @@ mutate_row_cb(const struct ovsdb_row *row, void *mr_)
     return *mr->error == NULL;
 }
 
+static bool
+count_row_cb(const struct ovsdb_row *row OVS_UNUSED, void *rc)
+{
+    size_t *row_count = rc;
+
+    (*row_count)++;
+
+    return true;
+}
+
 static struct ovsdb_error *
 ovsdb_execute_mutate(struct ovsdb_execution *x, struct ovsdb_parser *parser,
                      struct json *result)
@@ -609,7 +619,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 {
+            size_t row_count = 0;
+            ovsdb_query(table, &condition, count_row_cb, &row_count);
+            json_object_put(result, "count",
+                            json_integer_create(row_count));
+        }
+    } else if (!error) {
         mr.n_matches = 0;
         mr.txn = x->txn;
         mr.mutations = &mutations;

diff --git 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
@@ -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
@@ -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.