diff mbox series

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

Message ID 20240909045505.236657-6-mkp@redhat.com
State Accepted, archived
Commit 3d6b048d84d99ff71a6b6c15087ce28d90b22ac8
Delegated to: Eelco Chaudron
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 Sept. 9, 2024, 4:55 a.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.

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

Comments

Eelco Chaudron Sept. 10, 2024, 9:57 a.m. UTC | #1 
On 9 Sep 2024, at 6: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.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Simon Horman Sept. 11, 2024, 9:25 a.m. UTC | #2 
On Mon, Sep 09, 2024 at 12:55:02AM -0400, 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.
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

Acked-by: Simon Horman <horms@ovn.org>
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);