| Message ID | 20190302223825.11192-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | util/error: Trivial cleanup | expand |
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.
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.
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,
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(-)