Re: [Qemu-devel] [PATCH 2/2] util/error: Remove unnecessary saved_errno

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/3/19 2:12 AM, Eric Blake wrote:
> On 3/2/19 4:38 PM, Philippe Mathieu-Daudé wrote:
>> Since 552375088a8, error_set_errno() calls error_setv() which
>> already protect errno for clobbering.
>> Remove the now unnecessary saved_errno.
>>
>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  util/error.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/util/error.c b/util/error.c
>> index 934a78e1b1..0402fa1b9d 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -101,14 +101,11 @@ void error_setg_errno_internal(Error **errp,
>>                                 int os_errno, const char *fmt, ...)
>>  {
>>      va_list ap;
>> -    int saved_errno = errno;
>>  
>>      va_start(ap, fmt);
>>      error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap,
>>                 os_errno != 0 ? strerror(os_errno) : NULL);
>>      va_end(ap);
>> -
>> -    errno = saved_errno;
> 
> NACK. strerror() can clobber errno, so you still need to restore
> saved_errno, regardless of what error_setv() does internally.

Oops, I thought only strerror_r() would change errno, but checking the
man page I now see you are right:

  POSIX.1-2001 and POSIX.1-2008 require that a successful call to
  strerror() or strerror_l() shall leave errno unchanged, and note
  that, since no function return value is reserved to indicate an
  error, an application that wishes to check for errors should
  initialize errno to zero before the call, and then check errno
  after the call.

Thanks for catching that!

Phil.