[Qemu-devel] [PATCH 0/2] util/error: Trivial cleanup

[Qemu-devel] [PATCH 0/2] util/error: Trivial cleanup

[Philippe Mathieu-Daudé]

Trivial cleanups suggested by Daniel and Thomas.

Philippe Mathieu-Daudé (2):
  util/error: Remove an unnecessary NULL check
  util/error: Remove unnecessary saved_errno

 util/error.c | 7 -------
 1 file changed, 7 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] util/error: Remove an unnecessary NULL check

[Philippe Mathieu-Daudé]

This NULL check was required while introduced in 680d16dcb79f.
Later refactor added a NULL check in error_setv(), so this check
is now redundant.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/error.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/util/error.c b/util/error.c
index b5ccbd8eac..934a78e1b1 100644
--- a/util/error.c
+++ b/util/error.c
@@ -103,10 +103,6 @@ void error_setg_errno_internal(Error **errp,
     va_list ap;
     int saved_errno = errno;
 
-    if (errp == NULL) {
-        return;
-    }
-
     va_start(ap, fmt);
     error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap,
                os_errno != 0 ? strerror(os_errno) : NULL);
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

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;
 }
 
 void error_setg_file_open_internal(Error **errp,
-- 
2.20.1

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

[Eric Blake]

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.
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[Philippe Mathieu-Daudé]

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.

Re: [Qemu-devel] [PATCH 0/2] util/error: Trivial cleanup

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Trivial cleanups suggested by Daniel and Thomas.
>
> Philippe Mathieu-Daudé (2):
>   util/error: Remove an unnecessary NULL check
>   util/error: Remove unnecessary saved_errno

Queuing PATCH 1/2, thanks!