[ovs-dev] [PATCH] hmap.h: Fix incorrect expression

Usman S. Ansari ua1422 at gmail.com
Wed Mar 18 19:34:48 UTC 2020


I am only sending patch for the macro, for other I will send additional
patch(s)

On Wed, Mar 18, 2020 at 12:33 PM Usman Ansari <ua1422 at gmail.com> wrote:

> Will resubmit the patch.
>
> Coverity says 80 bugs fixed from this change.
>
> On Wed, Mar 18, 2020 at 12:19 PM William Tu <u9012063 at gmail.com> wrote:
>
> > On Wed, Mar 18, 2020 at 11:55 AM Ben Pfaff <blp at ovn.org> wrote:
> > >
> > > On Wed, Mar 18, 2020 at 11:43:29AM -0700, Usman Ansari wrote:
> > > > From Ben Pfaff blp at ovn.org
> > > >
> > > >
> > > > Coverity reports incorrect expression for HMAP_FOR_EACH_WITH_HASH
> macro
> > > > This patch fixes this issue
> > > > "make check" passes for this change
> > > > Coverity reports 80 errors resolved
> > > >
> > > > Signed-off-by: Usman Ansari <u1422 at gmail.com>
> > >
> > > Would you please rephrase the title of the patch?  There is nothing
> > > incorrect about the expression, it is just that Coverity complains
> about
> > > it.
> > >
> > > Thanks,
> > >
> > > Ben.
> >
> > Hi Usman,
> >
> > How about change the title to "hmap: Fix Coverity false positive."?
> >
> >     Coverity reports a false positive below:
> >     Incorrect expression, Assign_where_compare_meant: use of "="
> >     where "==" may have been intended.
> >     Fixed it by rewriting '(NODE = NULL)' as '((NODE = NULL), false)'.
> >
> > BTW, there are not only this place needed to fix. Please see diff below.
> > With this fix, you should see it fixes around 500 coverity defects.
> >
> > --- a/include/openvswitch/hmap.h
> > +++ b/include/openvswitch/hmap.h
> > @@ -136,12 +136,14 @@ struct hmap_node *hmap_random_node(const struct
> hmap
> > *);
> >   */
> >  #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)
>  \
> >      for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH),
> MEMBER); \
> > -         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > +         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > +         ((NODE = NULL), false); \
> >           ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),
>  \
> >                            MEMBER))
> >  #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)
>  \
> >      for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH),
> MEMBER); \
> > -         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > +         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > +         ((NODE = NULL), false); \
> >           ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER),
> > MEMBER))
> >
> >  static inline struct hmap_node *hmap_first_with_hash(const struct hmap
> *,
> > @@ -170,7 +172,8 @@ bool hmap_contains(const struct hmap *, const
> > struct hmap_node *);
> >      HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, (void) 0)
> >  #define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...)
>  \
> >      for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;
>  \
> > -         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > +         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > +         ((NODE = NULL), false); \
> >           ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
> MEMBER))
> >
> >  /* Safe when NODE may be freed (not needed when NODE may be removed from
> > the
> > @@ -179,7 +182,8 @@ bool hmap_contains(const struct hmap *, const
> > struct hmap_node *);
> >      HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, (void) 0)
> >  #define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)
> \
> >      for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;
>  \
> > -         ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL) \
> > +         ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > +         ((NODE = NULL), false) \
> >            ? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER),
> > MEMBER), 1 \
> >            : 0);
>  \
> >           (NODE) = (NEXT))
> > @@ -190,7 +194,8 @@ bool hmap_contains(const struct hmap *, const
> > struct hmap_node *);
> >  #define HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, ...)
> \
> >      for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
> > MEMBER), \
> >           __VA_ARGS__;
>  \
> > -         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > +         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > +         ((NODE = NULL), false); \
> >           ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
> MEMBER))
> >
> >  static inline struct hmap_node *
> > @@ -211,7 +216,8 @@ hmap_pop_helper__(struct hmap *hmap, size_t *bucket)
> {
> >  #define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP)
> >    \
> >      for (size_t bucket__ = 0;
> >    \
> >           INIT_CONTAINER(NODE, hmap_pop_helper__(HMAP, &bucket__),
> > MEMBER),  \
> > -         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL);)
> > +         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > +         ((NODE = NULL), false);)
> >
> >  static inline struct hmap_node *hmap_first(const struct hmap *);
> >  static inline struct hmap_node *hmap_next(const struct hmap *,
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list