[ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.

William Tu u9012063 at gmail.com
Wed Mar 18 19:58:47 UTC 2020


On Wed, Mar 18, 2020 at 8:36 AM Ben Pfaff <blp at ovn.org> wrote:
>
> On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
> > On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets <i.maximets at ovn.org> wrote:
> > >
> > > On 3/18/20 12:31 AM, William Tu wrote:
> > > > Coverity CID 279497 reports "Operands don't affect result".
> > > > Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> > > > is '0xffffff00'. So remove the statement.
> > > >
> > > > Cc: Usman Ansari <uansari at vmware.com>
> > > > Signed-off-by: William Tu <u9012063 at gmail.com>
> > > > ---
> > > >  lib/dpif-netdev.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > >
> > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > > index a798db45d9cb..0e2678d002d5 100644
> > > > --- a/lib/dpif-netdev.c
> > > > +++ b/lib/dpif-netdev.c
> > > > @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
> > > >          return EINVAL;
> > > >      }
> > > >
> > > > -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> > > > -        return EINVAL;
> > > > -    }
> > > > -
> > >
> > > I'm not sure if we need to remove this.  This code doesn't make any harm
> > > and most likely compiled out.  I agree that it doesn't change any logic
> > > in this function, but in case someone will try to add new flags or change
> > > the type of ct_state we will be safe and will reject all the unknown flags.
> > > Without this code we'll have to catch this case somehow on code review and
> > > re-introduce this check or implement missing functionality.
> > >
> > > One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> > > unused and should be removed along with _SUPPORTED_MASK.
> >
> > Good point.
> >
> > >
> > > So, I'd rather not touch this and just mark this code as OK for coverity
> > > scanner.  But if you want to remove, please, clean up other parts and
> > > add a build assert for the ct_state size and flags, so any disruptive change
> > > will be caught by the developer of this change.
> > >
> > OK thanks!
> > Let's keep this code block as it is now.
>
> I was surprised to hear that it doesn't have any effect.  Adding a
> comment might be helpful.

OK, then let me just add a comment.
In case in the future people run Coverity and see this again.
William


More information about the dev mailing list