diff mbox series

[ovs-dev,v2,5/8] classifier: Store n_indices between usage.

Message ID 20240820135549.1726748-6-mkp@redhat.com
State Changes Requested, archived
Delegated to: Ilya Maximets
Headers show
Series Address clang analyze warnings. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Mike Pattrick Aug. 20, 2024, 1:55 p.m. UTC 
Currently the Clang analyzer will complain about usage of an
uninitialized variable in the classifier. This is a false positive, but
not for a reason that could easily be detectable by clang.

The classifier is not safe for multiple writer threads to use
simultaneously so all callers protect these functions from simultaneous
writes. However, this is not so clear from the code's static analysis
alone. To help Clang out here, the n_indicies count is saved onto the
stack instead of accessed from the subtables struct repeatedly.

Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 lib/classifier.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Eelco Chaudron Aug. 30, 2024, 1:18 p.m. UTC | #1 
On 20 Aug 2024, at 15:55, Mike Pattrick wrote:

> Currently the Clang analyzer will complain about usage of an
> uninitialized variable in the classifier. This is a false positive, but
> not for a reason that could easily be detectable by clang.
>
> The classifier is not safe for multiple writer threads to use
> simultaneously so all callers protect these functions from simultaneous
> writes. However, this is not so clear from the code's static analysis
> alone. To help Clang out here, the n_indicies count is saved onto the
> stack instead of accessed from the subtables struct repeatedly.
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

This change looks good to me.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
diff mbox series

Patch

diff --git a/lib/classifier.c b/lib/classifier.c
index 0729bd190..55f23b976 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -527,6 +527,7 @@  classifier_replace(struct classifier *cls, const struct cls_rule *rule,
     struct cls_match *head;
     unsigned int mask_offset;
     size_t n_rules = 0;
+    uint8_t n_indices;
     uint32_t basis;
     uint32_t hash;
     unsigned int i;
@@ -543,7 +544,8 @@  classifier_replace(struct classifier *cls, const struct cls_rule *rule,
     /* Compute hashes in segments. */
     basis = 0;
     mask_offset = 0;
-    for (i = 0; i < subtable->n_indices; i++) {
+    n_indices = subtable->n_indices;
+    for (i = 0; i < n_indices; i++) {
         ihash[i] = minimatch_hash_range(&rule->match, subtable->index_maps[i],
                                         &mask_offset, &basis);
     }
@@ -575,7 +577,7 @@  classifier_replace(struct classifier *cls, const struct cls_rule *rule,
         }
 
         /* Add new node to segment indices. */
-        for (i = 0; i < subtable->n_indices; i++) {
+        for (i = 0; i < n_indices; i++) {
             ccmap_inc(&subtable->indices[i], ihash[i]);
         }
         n_rules = cmap_insert(&subtable->rules, &new->cmap_node, hash);
@@ -713,6 +715,7 @@  classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
     struct cls_subtable *subtable;
     uint32_t basis = 0, hash, ihash[CLS_MAX_INDICES];
     unsigned int mask_offset;
+    uint8_t n_indices;
     size_t n_rules;
     unsigned int i;
 
@@ -730,7 +733,8 @@  classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
     ovs_assert(subtable);
 
     mask_offset = 0;
-    for (i = 0; i < subtable->n_indices; i++) {
+    n_indices = subtable->n_indices;
+    for (i = 0; i < n_indices; i++) {
         ihash[i] = minimatch_hash_range(&cls_rule->match,
                                         subtable->index_maps[i],
                                         &mask_offset, &basis);
@@ -783,7 +787,7 @@  classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
     }
 
     /* Remove rule node from indices. */
-    for (i = 0; i < subtable->n_indices; i++) {
+    for (i = 0; i < n_indices; i++) {
         ccmap_dec(&subtable->indices[i], ihash[i]);
     }
     n_rules = cmap_remove(&subtable->rules, &rule->cmap_node, hash);