Re: [PATCH 05/11] eal: force prefix for internal threads

[Thomas Monjalon <thomas@monjalon.net>]

07/09/2023 13:10, Morten Brørup:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 7 September 2023 10.55
> > 
> > On Thu, Sep 7, 2023 at 10:53 AM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Thu, Sep 7, 2023 at 10:50 AM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > > > > This 10 value in the comment is easy to miss if some change with the
> > > > > prefix is done.
> > > > > Mentionning RTE_THREAD_INTERNAL_NAME_SIZE is enough.
> > > >
> > > > I disagree with David's comment to this.
> > > >
> > > > The function documentation is easier to read if the actual number is also
> > mentioned.
> > > >
> > > > For the best of both worlds, you can add something like this nearby:
> > > >
> > > > _Static_assert(sizeof(RTE_THREAD_NAME_PREFIX) == sizeof("dpdk-"),
> > > >                 "Length of RTE_THREAD_NAME_PREFIX has changed; "
> > > >                 "the documentation needs updating.");
> > >
> > > And how will it catch the comment about 10 characters ?
> > 
> > I mean you still have to re-read the whole documentation and look for
> > some reference somewhere about 10 characters.
> 
> The trick is to put the _Static_assert close to where the expectation occurs. That makes it easier to find where changes are necessary.
> 
> And the _Static_assert can be added at all the locations where changes would be necessary. (Generally, we should add a lot more _Static_assert to the code where it makes assumptions about e.g. the ordering of fields in a struct, such as the vector optimized code.)
> 
> Also, the failure message could be improved to include help about what to look for.
> 
> PS: The reference to RTE_THREAD_INTERNAL_NAME_SIZE should remain in the documentation, so perhaps look for "RTE_THREAD_INTERNAL_NAME_SIZE".

I agree with David, it is easier to maintain if not mentioning
the exact value in the doc,
and having a mention to RTE_THREAD_INTERNAL_NAME_SIZE is enough
if it defined as "11".
Then we need only one static assert (or RTE_BUILD_BUG_ON)
below the definition to make sure the number is still valid.