On Wed, Mar 18, 2020 at 04:53:27PM +0100, Ilya Maximets wrote:
> On 3/18/20 4:36 PM, Ben Pfaff 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.maxim...@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 <uans...@vmware.com>
> >>>> Signed-off-by: William Tu <u9012...@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.
> >
>
> DP_NETDEV_CS_UNSUPPORTED_MASK was introduced at the time when dpif-netdev
> didn't
> support NAT. After the NAT support implementation in commit
> 4cddb1f0d837 ("dpdk: Parse NAT netlink for userspace datapath.") this mask is
> just
> a zero in the lowest 8 bits, i.e. all current flags are supported.
>
> I'm not sure why it's casted to uint32_t, though, or why flow->ct_state is 8
> bits
> only. packets.h has similar mask and it's casted to uint32_t too. The main
> concern
> here is that it seems like ct_state is 32bit in netlink. That produces
> misunderstanding
> and makes me nervous about potential issues.
>
> flow->ct_state is 8 bits long and mask is zero there, so this 'if' statement
> is
> always false.
Oh, I understand the reason, but from glancing at the code it's not
obvious.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
