Re: [PATCH v2] Fix bug and add osnoise_trace_is_off()

[Dan Carpenter <dan.carpenter@linaro.org>]

You need an "rtla: " subsystem prefix in the subject.  You're going to
need to remove the words "fix" and "bug" from the subject because this
is just a cleanup.

On Wed, Jan 15, 2025 at 10:09:56AM +0200, Costa Shulyupin wrote:
> The usage of trace_is_off() contains a small and elusive
> bug that requires a detailed explanation.
> 
> To expose the bug, let's modify the source code by moving the first member,
> `trace`, of the `osnoise_tool` structure to the second position:
> 
>  struct osnoise_tool {
> -       struct trace_instance           trace;
>         struct osnoise_context          *context;
> +       struct trace_instance           trace;
> 
> A correct program would work properly after this change,
> but this one does not.

No...

You introduced a bug by changing the order.

You have to undestand that to the original authors this stuff was really
easy and they knew the order of the struct members because they chose it
deliberately.  In the end, they get so used to the code that
"&record->trace" just becomes an idiom for casting "record" and they
forget how it looks to a newcomer.

I *personally* am not a fan of code which assumes we know the order of
the struct members so I don't have a problem with you re-writing the
code.  But the commit message must say that it is just a cleanup and not
a fix.

Which reminds me that I had intended to create a container_of_first()
for code like this which assumes that container_of() is just a cast.
There is lots of code like this:

	struct something *member = container_of(p, struct foo, first_member);

	if (IS_ERR(member)) {

Which relies on the face that "first_member" is the first member of
foo struct.  It's a quite common thing.

regards,
dan carpenter