Re-explaining that errno thing....

Jeremy Allison jra at samba.org
Mon Feb 27 00:32:30 UTC 2017


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.

Jeremy.



More information about the samba-technical mailing list