On 10/16/20 3:44 PM, Thomas Gleixner wrote:
> 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.
It wasn't pure luck, that was checked before that change was made. No
users did anything funky, it was all false/true or 0/1.
> Please spell it out correctly that converting a boolean argument to an
> integer argument is not equivalent.
Fixed up the commit message to be more descriptive.
>> 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.
I added a WARN_ON_ONCE() in the default case for that one.
>> - * @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 really is nothing wrong, but it's not very descriptive (wasn't
before either). I've added a fuller description of the various TWA_*
notification types now.
--
Jens Axboe
