Re-explaining that errno thing....

[Michael Adam]

On 2017-02-26 at 16:32 -0800, Jeremy Allison wrote:
> On Sun, Feb 26, 2017 at 07:28:19AM +0200, Uri Simchoni wrote:
> > Jeremy,
> > 
> > I thought I understood commit cda6764f1a8db96182bfd1855440bc6a1ba1abee,
> > which is about correctness of errno in failures in vfs_shadow_copy2, but
> > yesterday, when reviewing Ralph / Metze's vfs_catia patch, I found out
> > that I can't explain to myself why always saving errno and restoring it
> > is incorrect (there's this pattern in that patch).
> > 
> > So, commit cda6764f1a8db96182bfd1855440bc6a1ba1abee says that the way to
> > deal with errors is:
> > 
> > 	int save_errno = 0;
> > 	
> > 	if (error) {
> > 		save_errno = errno;
> > 		goto out;
> > 	}
> > 
> > out:
> > ...do some cleanup
> > 	if (save_errno) {
> > 		errno = save_errno;
> > 	}
> > 	return ret;
> > 
> > 
> > Whereas the previous pattern was:
> > 
> > 	int save_errno;
> > 	
> > 	if (error) {
> > 		goto out;
> > 	}
> > 
> > out:
> > 	save_errno = errno;
> > ...do some cleanup
> > 	errno = save_errno;
> > 	return ret;
> > 
> > 
> > The commit said code shouldn't be *setting* errno if there's no error.
> > Fine. But the "old" pattern isn't setting errno - it's keeping it the
> > way it was.
> > 
> > The "old" pattern is less obviously correct, as errno has to be kept
> > untouched from the failure point up to the start of cleanup. But it
> > seems to me like the pattern itself doesn't break anything (it just lets
> > code around it break things).
> > 
> > Is that analysis correct or am I missing something again?
> 
> Errr. Probably not, it's just the the 'old' pattern *looks*
> much more dangerous to me :-).
> 
> I really don't like to touch something - especially something
> that's a global (or thread-local storage) unless I know I need
> to change it.
> 
> So depending on the code surrounding it, the "old" pattern
> probably is safe, it's just that IMHO new code should be
> using the new pattern, as that is much clearer on what is
> being changed when.
> 
> It's a matter or continually raising the code quality IMHO,
> and to me, pattern #1 is better.

I personally find the old pattern more obvious, in the sense
that the errno bracket around the cleanup code is much more
obvious.

But it has 2 issues:

1) As Uri said, the erroring-out code must make sure that
   between the failed call and the goto out, there is no
   errno-changing call.
2) The way it is written, it may be setting errno in cases
   where it would not be needed (errno == 0).
   This could of course be fixed with adding if (saved_errno) ...
   as in the old pattern.
   On the other hand side, this may be desired if for instance
   the failing operation did not produce an errno, but the
   cleanup code did set an errno... Not sure what's better
   in this situation.

The new pattern has issues:

1) Because errno handling is scattered, it duplicates code
   and is less obvious (to me). And the programmer must
   always remember to save the errno at each failure point.
2) The reverse of issue #2 above: If there was no errno
   in the originally failing code, but there is an errno
   in the cleanup code, then the cleanup code's errno will
   be set at exit. That may or may not be desired.

So, I'd say neither approach is perfect. I personally
tend to prefer a variant of the old pattern.

My 2 cents..

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170227/dfc81bc4/signature.sig>