Re: [PATCH] aoe: Use IS_ERR_OR_NULL() to clean code

[Al Viro]

On Fri, Aug 23, 2024 at 01:26:40PM +0800, zhangjiao2 wrote:
> From: Zhang Jiao <zhangjiao2@xxxxxxxxxxxxxxxxxxxx>
> 
> Use IS_ERR_OR_NULL() to make the code cleaner.

ITYM "obfuscate the bogus code".

Take a look at kthread_run() definition:

#define kthread_run(threadfn, data, namefmt, ...)                          \
({                                                                         \
        struct task_struct *__k                                            \
                = kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
        if (!IS_ERR(__k))                                                  \
                wake_up_process(__k);                                      \
        __k;                                                               \
})

OK, what would need to happen for that to return NULL?  kthread_create()
returning NULL *AND* wake_up_process(NULL) surviving, right?

int wake_up_process(struct task_struct *p)
{
        return try_to_wake_up(p, TASK_NORMAL, 0);
}

OK, so we'd need try_to_wake_up(NULL, ...) to survive execution:

int try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
{
        guard(preempt)();
        int cpu, success = 0;

        if (p == current) {
		whatever, current is never NULL or a lot of places would be
		utterly screwed
	}
	/* some comment */
        scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {

... and that would start with trying to grab &NULL->pi_lock, which is not
going to survive.

>  	task = kthread_run(kthread, k, "%s", k->name);
> -	if (task == NULL || IS_ERR(task))
> +	if (IS_ERR_OR_NULL(task))
>  		return -ENOMEM;

In other words, task == NULL had been pointless all along.  Your change only
makes it harder to spot.

IS_ERR_OR_NULL is almost never the right thing to do; there are cases where
a function may legitimately return a pointer to object, NULL *or* ERR_PTR(something),
but most of the time it's either impossible (and the caller couldn't have been
arsed to check what the calling conventions are) or a sign of a function in
bad need of saner calling conventions.

In this case it's the former - kthread_run() never returns a NULL and
actually if you look into kthread_create() you'll see that it returns
a pointer to new task_struct instance on success and ERR_PTR(-E...) on
failure.  NULL is never returned by that thing.

This kind of "defensive" programming only confuses the readers; please,
don't paper over that garbage.