ksummit.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Hannes Reinecke <hare@suse.de>
To: Julia Lawall <julia.lawall@inria.fr>,
	Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Arnd Bergmann <arnd@arndb.de>, NeilBrown <neilb@suse.de>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	ksummit@lists.linux.dev
Subject: Re: Potential static analysis ideas
Date: Mon, 26 Jul 2021 11:35:47 +0200	[thread overview]
Message-ID: <45ed0e03-2eb8-8ed8-898b-4f5f4d31a4a1@suse.de> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2107261125380.6460@hadrien>

On 7/26/21 11:28 AM, Julia Lawall wrote:
>>>> Also, over 230 files contain functions that return both NULL and ERR_PTR.
>>>> A random example, chosen for conciseness, is the following from
>>>> fs/overlayfs/inode.c:
>>>>
>>>> struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>>>>                                  bool is_upper)
>>>> {
>>>>        struct inode *inode, *key = d_inode(real);
>>>>
>>>>           inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
>>>>        if (!inode)
>>>>                   return NULL;
>>>>
>>>>           if (!ovl_verify_inode(inode, is_upper ? NULL : real,
>>>>                                 is_upper ? real : NULL, false)) {
>>>>                   iput(inode);
>>>>                   return ERR_PTR(-ESTALE);
>>>>           }
>>>>
>>>>           return inode;
>>>> }
>>>>
>>> And that I would consider a coding error.
>>> If a function is able to return an error pointer it should _always_
>>> return an error pointer; here it would be trivial to return -ENXIO
>>> instead of NULL in the first condition.
>>>
>>> Not doing so is just sloppy programming IMO.
>>
>> In this case I agree.
> 
> I looked at the calling context, and it is not so obvious.  There are
> different behaviors in the two cases at both callsites.  It is like what
> Dan describes.  If the thing is not available, we just move on.  If it is
> available some action is needed.  If there is an actual error, some error
> handling is needed.
> 
But isn't 'not available' an error, too?
And isn't that exactly why we have individual errnos?

Why do we have to introduce different classes of errors (errno, NULL 
pointer), when we could have used a simple errno lik -ENXIO?
And, of course, documentation for that function outlining what exactly 
the meaning of the individual error numbers is.
But then we would need that anyway to clarify how the caller should 
interpret a 'NULL' return value.

So: not convinced :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

  reply	other threads:[~2021-07-26  9:35 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 19:10 Potential static analysis ideas Dan Carpenter
2021-07-24 13:33 ` Geert Uytterhoeven
2021-07-24 13:40   ` Julia Lawall
2021-07-24 14:08   ` Arnd Bergmann
2021-07-24 23:18   ` Laurent Pinchart
2021-07-24 23:45     ` NeilBrown
2021-07-26  7:25       ` Arnd Bergmann
2021-07-26  7:53         ` Geert Uytterhoeven
2021-07-26  8:20           ` Arnd Bergmann
2021-07-26  8:39             ` Geert Uytterhoeven
2021-07-26  8:52               ` Arnd Bergmann
2021-07-26  9:11                 ` Geert Uytterhoeven
2021-07-26  8:55             ` Julia Lawall
2021-07-26  9:08               ` Hannes Reinecke
2021-07-26  9:16                 ` Geert Uytterhoeven
2021-07-26  9:28                   ` Julia Lawall
2021-07-26  9:35                     ` Hannes Reinecke [this message]
2021-07-26 10:03                       ` Julia Lawall
2021-07-26 17:54                   ` James Bottomley
2021-07-26 18:16                     ` Linus Torvalds
2021-07-26 21:53                       ` NeilBrown
2021-07-26 18:31                     ` Laurent Pinchart
2021-07-26  9:17                 ` Dan Carpenter
2021-07-26  9:13             ` Dan Carpenter
2021-07-26 21:43         ` NeilBrown
2021-07-26  7:05   ` Dan Carpenter
2021-07-26 15:50 ` Paul E. McKenney
2021-07-27  9:38   ` Dan Carpenter
2021-07-27  9:50     ` Geert Uytterhoeven
2021-07-27 16:06     ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45ed0e03-2eb8-8ed8-898b-4f5f4d31a4a1@suse.de \
    --to=hare@suse.de \
    --cc=arnd@arndb.de \
    --cc=dan.carpenter@oracle.com \
    --cc=geert@linux-m68k.org \
    --cc=julia.lawall@inria.fr \
    --cc=ksummit@lists.linux.dev \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=neilb@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).