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