[ovs-dev] [PATCH v4 1/7] dpif-netdev: implement subtable lookup validation.

Van Haaren, Harry harry.van.haaren at intel.com
Tue Jun 30 11:37:13 UTC 2020


> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Tuesday, June 30, 2020 11:01 AM
> To: William Tu <u9012063 at gmail.com>
> Cc: ovs-dev <ovs-dev at openvswitch.org>; Stokes, Ian <ian.stokes at intel.com>; Ilya
> Maximets <i.maximets at ovn.org>; Federico Iezzi <fiezzi at redhat.com>
> Subject: RE: [PATCH v4 1/7] dpif-netdev: implement subtable lookup validation.
> 
> > -----Original Message-----
> > From: William Tu <u9012063 at gmail.com>
> > Sent: Saturday, June 27, 2020 7:27 PM
> > To: Van Haaren, Harry <harry.van.haaren at intel.com>
> > Cc: ovs-dev <ovs-dev at openvswitch.org>; Stokes, Ian <ian.stokes at intel.com>;
> > Ilya Maximets <i.maximets at ovn.org>; Federico Iezzi <fiezzi at redhat.com>
> > Subject: Re: [PATCH v4 1/7] dpif-netdev: implement subtable lookup validation.
> >
> > On Thu, Jun 18, 2020 at 9:53 AM Harry van Haaren
> > <harry.van.haaren at intel.com> wrote:

<snip other topics>

> > > - Add check to ensure autovalidator is 0th item in lookup array
> >
> > What's your concern here? Users can only change the prio, not
> > the .probe function. So autovalidator is always at 0th item.
> > If you worry about code being changed accidentally in the future,
> > how about using BUILD_ASSERT_DECL?
> 
> Yes - good improvement.

Indeed the goal is to ensure the autovalidator is the 0th lookup, and
that code is not changed accidentally. It seems like using a BUILD_ASSERT_DECL
would achieve that, however the compiler doesn't agree with me:

lib/dpif-netdev-lookup.c:67:45: error: expression in static assertion is not constant
 BUILD_ASSERT_DECL(subtable_lookups[0].probe == dpcls_subtable_autovalidator_probe);
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~

In this case, the subtable_lookups[] is static, but not const. It cannot be const as
the user must be able to change the .priority field of each lookup implementation.
Doing a build time assert is (correctly) flagging that we're asserting non-const data,
which could be changed during runtime.

The autovalidator component has a runtime check using ovs_assert() today, so a
check is already being done:
    ovs_assert(lookup_funcs[0].probe(0, 0) == dpcls_subtable_autovalidator);

Summary:
- The existing check in autovalidator is enough to ensure its element 0 in the lookups array.
- Adding a BUILD_ASSERT_DECL isn't allowed by the compiler, as its not const data being validated.



More information about the dev mailing list