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

[Cong Ding]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/