Re: [PATCH] task_work: cleanup notification modes

[Thomas Gleixner]

On Fri, Oct 16 2020 at 09:16, Jens Axboe wrote:
> A previous commit changed the notification mode from 0/1 to allowing

No. It changed it from boolean to an int.

There is a fundamental difference between 0/1 and false/true simply
because it's a compiler implementation detail how to represent a boolean
value.

Assume the following:

#define BAZ	0x08

       task_work_add(tsk, &work, foo & BAZ);

So with a function signature of

       task_work_add(*tsk, *work, bool signal);

the above 'foo & BAZ' becomes either true of false.

With the changed function signature of

       task_work_add(*tsk, *work, int signal);

the above becomes the result of 'foo & BAZ' which means that this
construct will not longer do the right thing.

It's pure luck that none of the usage sites relied on the boolean
property of that argument.

Please spell it out correctly that converting a boolean argument to an
integer argument is not equivalent.

>  	switch (notify) {
> +	case TWA_NONE:
> +		break;
>  	case TWA_RESUME:
>  		set_notify_resume(task);
>  		break;

The enum will not prevent that either and what you really want to do is
to have some form of debug warning if 'notify' is out of range, which
would have been the right thing to do in the first place.

> - * @notify: send the notification if true
> + * @notify: send chosen notification, if any

Is that really all you found to be wrong in that comment?

There is more and please take your time which will in turn save time and
nerves on both ends.

Thanks,

        tglx