Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 07/28/2016 12:19 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 07/27/2016 03:24 AM, Sascha Silbe wrote:
>>> C11 allows errno to be clobbered by pretty much any library function
>>> call, so in general callers need to take care to save errno before
>>> calling other functions.
>>>
>>> However, for error reporting functions this is rather awkward and can
>>> make the code on the caller side more complicated than
>>> necessary. error_setg_errno() already takes care of preserving errno
>>> and some functions rely on that, so just promise that we continue to
>>> do so in the future.
>>>
>>> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
>>> ---
>>>
>>> Alternative approach to "error: error_setg_errno(): errno may be
>>> clobbered" [1].
>>
>> I like this alternative better.
>>
>>
>>> +++ b/include/qapi/error.h
>>> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
>>>   * Just like error_setg(), with @os_error info added to the message.
>>>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>>>   * the human-readable error message.
>>> + *
>>> + * The value of errno (which usually can get clobbered by almost any
>>> + * function call) will be preserved.
>>>   */
>>>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>>>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>>
>> Do we need/want to make the guarantee of preserving errno across any of
>> the other functions and macros declared in error.h?
> 
> I guess we should for the ones that preserve errno, to make that
> preservation actually useful.  These are: error_setv(),
> error_setg_errno_internal(), error_append_hint().  Indirectly:
> error_set_internal(), error_set(), error_setg_internal(), error_setg(),
> error_setg_file_open_internal(), error_setg_file_open(), possibly
> error_setg_win32_internal() and error_setg_win32().
> 

The implementation of preserve errno seems inconsistent to me.  The
function error_setv is static, and I guess it is supposed to provide
this indirect errno preservation and is used for both error_setg und
error_setg_errno, yet error_setg_ errno_internal does extra save-restore
itself while error_setg_iternal relies on 'indirect', what is not OK in
my opinion.

As Sascha pointed out, in C11 any library functions may change errno
unless explicitly told otherwise for the particular function.  Since
start_va and end_va has nothing on preserving errno it is guaranteed by
the standard that they persevere errno, and we should assume they don't.

I could prepare a patch for this. Should I?

Halil