LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Cornelia Huck <cornelia.huck@de.ibm.com>
To: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
Cc: linux-kernel@vger.kernel.org, gregkh@suse.de, akpm@linux-foundation.org
Subject: Re: [PATCH debugfs: implement symbolic links
Date: Tue, 13 Feb 2007 16:45:51 +0100	[thread overview]
Message-ID: <20070213164551.500cd390@gondolin.boeblingen.de.ibm.com> (raw)
In-Reply-To: <45D19D72.4050009@de.ibm.com>

On Tue, 13 Feb 2007 12:13:54 +0100,
Peter Oberparleiter <peter.oberparleiter@de.ibm.com> wrote:

Not especially related to this patch (which just does the same as the
other debugfs functions), but:

> + * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> + * returned.  It is not wise to check for this value, but rather, check for
> + * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
> + * code.

does not look like good advice for return code handling. Return code
seems to be:

- ERR_PTR(-ENODEV) if debugfs is disabled
- NULL if debugfs is enabled and something went wrong
- !NULL and !IS_ERR if debugfs is enabled and all went fine

That makes it easy to get return code checking wrong (especially
considering the comment above), and a number of callers do get it wrong.

How about changing the return code behaviour of the debugfs code, either

1. return NULL if debugfs is disabled or something went wrong, !NULL
   else or
2. return ERR_PTR(-ENODEV) if debugfs is disabled, ERR_PTR(-ESOMEERROR)
   if something went wrong or a proper dentry if everything went fine?

At the very least we should change the misleading comment.

-- 
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: cornelia.huck@de.ibm.com

  reply	other threads:[~2007-02-13 15:44 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-13 11:13 Peter Oberparleiter
2007-02-13 15:45 ` Cornelia Huck [this message]
2007-02-14  1:31   ` Greg KH
2007-02-14  6:42     ` Cornelia Huck
2007-02-14  6:57       ` [Patch] debugfs: Remove misleading comments Cornelia Huck
2007-02-14  1:27 ` [PATCH debugfs: implement symbolic links Greg KH
2007-02-14  8:03   ` Peter 1 Oberparleiter
2007-02-14 19:45     ` Greg KH

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=20070213164551.500cd390@gondolin.boeblingen.de.ibm.com \
    --to=cornelia.huck@de.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=gregkh@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peter.oberparleiter@de.ibm.com \
    --subject='Re: [PATCH debugfs: implement symbolic links' \
    /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

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).