Re-explaining that errno thing....

[Uri Simchoni]

Thanks for the clarification. I will try to follow this guideline myself :)

Uri

On 02/27/2017 02:32 AM, 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.
> 
> Jeremy.
>