On Tue, Jun 30, 2020 at 4:37 AM Van Haaren, Harry
<[email protected]> wrote:
>
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Tuesday, June 30, 2020 11:01 AM
> > To: William Tu <[email protected]>
> > Cc: ovs-dev <[email protected]>; Stokes, Ian <[email protected]>;
> > Ilya
> > Maximets <[email protected]>; Federico Iezzi <[email protected]>
> > Subject: RE: [PATCH v4 1/7] dpif-netdev: implement subtable lookup
> > validation.
> >
> > > -----Original Message-----
> > > From: William Tu <[email protected]>
> > > Sent: Saturday, June 27, 2020 7:27 PM
> > > To: Van Haaren, Harry <[email protected]>
> > > Cc: ovs-dev <[email protected]>; Stokes, Ian <[email protected]>;
> > > Ilya Maximets <[email protected]>; Federico Iezzi <[email protected]>
> > > Subject: Re: [PATCH v4 1/7] dpif-netdev: implement subtable lookup
> > > validation.
> > >
> > > On Thu, Jun 18, 2020 at 9:53 AM Harry van Haaren
> > > <[email protected]> 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.
>
Thanks for trying it out. Then the existing checking way looks ok.
Thank you
William
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
