linux-kernel - Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121201095831.GA18966@gmail.com>
Date:	Sat, 1 Dec 2012 09:58:32 +0000
From:	Cong Ding <dinggnu@...il.com>
To:	"Hans J. Koch" <hjk@...sjkoch.de>
Cc:	Vitalii Demianets <vitas@...factor.kiev.ua>,
	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized

On Sat, Dec 01, 2012 at 04:56:25AM +0100, Hans J. Koch wrote:
> On Sat, Dec 01, 2012 at 02:22:44AM +0100, Cong Ding wrote:
> > On Fri, Nov 30, 2012 at 10:33 PM, Hans J. Koch <hjk@...sjkoch.de> wrote:
> > > On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote:
> > >> I like Vitalii's solution more. Hans's solution assign the value
> > >> -ENOMEM to ret in every round of the loop, which is a kind of wasting
> > >> CPU cycles.
> > >
> > > The difference between
> > > 1 files changed, 12 insertions(+), 4 deletions(-) and
> > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > is more important. Note that this code is not in a fast path but only
> > > called once at device creation.
> > Why do you think the size of the patch is so important? I think the
> > most important thing is the coding style and efficiency. I agree
> > efficiency is not important in this case, but what about coding style?
> 
> Most important. Short is beautiful.
> 
> > Your code is _not_ very easy to understand.
> 
> You think so?
> 
> > It's a very natural thing
> > to set the return value and then goto the error-handling codes, which
> > is exactly same as what Vitalii did, rather than setting an initial
> > value in the beginning of each round of the loop as you did.
> 
> Setting a default value at the beginning of a block is the most natural
> thing. I don't want to repeat the same code in three places.
> 
> > There are
> > also a bunch of codes in kernel in the same style with Vitalii, but I
> > cannot find anything same as your codes.
> 
> If you follow LKML closely, you'll notice that simplifying code by
> replacing unnecessary repetitions with shorter versions is always welcome.
> If we didn't go for that, the kernel source would be a few million lines
> bigger by now.
> 
> Thanks,
> Hans
If it is really necessary to save the 4 lines of codes, I would suggest to do 
in the following style. But you are more senior than me, so I may be wrong in 
this aspect. 

 drivers/uio/uio.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..cb20168 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -263,7 +263,7 @@ static struct class uio_class = {
  */
 static int uio_dev_add_attributes(struct uio_device *idev)
 {
-	int ret;
+	int ret = 0;
 	int mi, pi;
 	int map_found = 0;
 	int portio_found = 0;
@@ -339,6 +339,8 @@ err_map:
 		kobject_put(&map->kobj);
 	}
 	kobject_put(idev->map_dir);
+	if (!ret)
+		ret = -ENOMEM;
 	dev_err(idev->dev, "error creating sysfs files (%d)\n", ret);
 	return ret;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/