Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Matt Mackall]

On Tue, 2008-01-15 at 11:10 -0800, Randy Dunlap wrote:
> On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:
> 
> > 2008/1/15, Christoph Hellwig <hch@xxxxxxxxxxxxx>:
> > > On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
> 
> > > > @@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
> > > >       unsigned long end;
> > > >       struct mm_struct *mm = current->mm;
> > > >       struct vm_area_struct *vma;
> > > > -     int unmapped_error = 0;
> > > > -     int error = -EINVAL;
> > > > +     int error = 0, unmapped_error = 0;
> > > >
> > > >       if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> > > > -             goto out;
> > > > +             return -EINVAL;
> > > >       if (start & ~PAGE_MASK)
> > > > -             goto out;
> > > > +             return -EINVAL;
> > >
> > > The goto out for a simple return style is used quite commonly in kernel
> > > code to have a single return statement which makes code maintaince, e.g.
> > > adding locks or allocations simpler.  Not sure that getting rid of it
> > > makes a lot of sense.
> > 
> > Sorry, I can't agree. That's what is written in the CodingStyle document:
> > 
> > The goto statement comes in handy when a function exits from multiple
> > locations and some common work such as cleanup has to be done.
> 
> CodingStyle does not try to cover Everything.  Nor do we want it to.
> 
> At any rate, there is a desire for functions to have a single point
> of return, regardless of the amount of cleanup to be done, so I agree
> with Christoph's comments.

When we're not cleaning up resources, the main advantage of having a
single point of return is that you can trace backwards from the return
point through the function's logic. But that advantage flies right out
the window when you use gotos. You still have to figure out how you got
to the return statement by tracing back and looking at all the possible
gotos. And the "goto out" style adds bulk and non-negligible complexity
when we've got to search back for what the last explicitly set value of
"ret" or "error" or whatever the function in question is using was.
Sometimes people get this wrong ("retval is already -EINVAL, so I don't
need to explicitly set it"), and create bugs.

So I think if we're not actually going to use "structured
programming" (no gotos) or "stack cleanup" styles, the single return
point style is more trouble than it's worth.

A lesser advantage of the single return point is that you can set a
breakpoint or put a printk at the end of a function. But I don't think
that's much justification.

-- 
Mathematics is the supreme nostalgia of our time.

--
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/