[Qemu-devel] [PATCH] util/error: Save errno from clobbering

[Qemu-devel] [PATCH] util/error: Save errno from clobbering

[Max Reitz]

There may be calls to error_setg() and especially error_setg_errno()
which blindly (and until now wrongly) assume these functions not to
clobber errno (e.g., they pass errno to error_setg_errno() and return
-errno afterwards). Instead of trying to find and fix all of these
constructs, just make sure error_setg() and error_setg_errno() indeed do
not clobber errno.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 util/error.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/util/error.c b/util/error.c
index ec0faa6..3ee362a 100644
--- a/util/error.c
+++ b/util/error.c
@@ -27,6 +27,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
 {
     Error *err;
     va_list ap;
+    int saved_errno = errno;
 
     if (errp == NULL) {
         return;
@@ -41,6 +42,8 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     err->err_class = err_class;
 
     *errp = err;
+
+    errno = saved_errno;
 }
 
 void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
@@ -49,6 +52,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
     Error *err;
     char *msg1;
     va_list ap;
+    int saved_errno = errno;
 
     if (errp == NULL) {
         return;
@@ -69,6 +73,8 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
     err->err_class = err_class;
 
     *errp = err;
+
+    errno = saved_errno;
 }
 
 void error_setg_file_open(Error **errp, int os_errno, const char *filename)
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH] util/error: Save errno from clobbering

[Benoît Canet]

Le Thursday 07 Nov 2013 à 20:10:29 (+0100), Max Reitz a écrit :
> There may be calls to error_setg() and especially error_setg_errno()
> which blindly (and until now wrongly) assume these functions not to
> clobber errno (e.g., they pass errno to error_setg_errno() and return
> -errno afterwards). Instead of trying to find and fix all of these
> constructs, just make sure error_setg() and error_setg_errno() indeed do
> not clobber errno.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  util/error.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/util/error.c b/util/error.c
> index ec0faa6..3ee362a 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -27,6 +27,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>  {
>      Error *err;
>      va_list ap;
> +    int saved_errno = errno;
>  
>      if (errp == NULL) {
>          return;
> @@ -41,6 +42,8 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      err->err_class = err_class;
>  
>      *errp = err;
> +
> +    errno = saved_errno;
>  }
>  
>  void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> @@ -49,6 +52,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>      Error *err;
>      char *msg1;
>      va_list ap;
> +    int saved_errno = errno;
>  
>      if (errp == NULL) {
>          return;
> @@ -69,6 +73,8 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>      err->err_class = err_class;
>  
>      *errp = err;
> +
> +    errno = saved_errno;
>  }
>  
>  void error_setg_file_open(Error **errp, int os_errno, const char *filename)
> -- 
> 1.8.4.2
> 
> 

Yes this look better than trying to fix all callers.

Reviewed-by: Benoit Canet <benoit@irqsave.net

Re: [Qemu-devel] [PATCH] util/error: Save errno from clobbering

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

On 11/07/2013 12:10 PM, Max Reitz wrote:
> There may be calls to error_setg() and especially error_setg_errno()
> which blindly (and until now wrongly) assume these functions not to
> clobber errno (e.g., they pass errno to error_setg_errno() and return
> -errno afterwards). Instead of trying to find and fix all of these
> constructs, just make sure error_setg() and error_setg_errno() indeed do
> not clobber errno.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  util/error.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

I did a quick glance through 'git grep -p -A2 error_set' for any more
culprits beyond the one you just fixed in block.c, and didn't spot any
obvious ones; but there were enough 'goto error' statements where I
didn't check if the code at the error label was depending on sane errno
value; and it is definitely easier for code maintenance if you don't
have to think about whether logging an error will clobber the error.
(Not to mention I've also patched libvirt to have the same paradigm of
logging functions guaranteeing no modification to errno).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] util/error: Save errno from clobbering

[Stefan Hajnoczi]

On Thu, Nov 07, 2013 at 08:10:29PM +0100, Max Reitz wrote:
> There may be calls to error_setg() and especially error_setg_errno()
> which blindly (and until now wrongly) assume these functions not to
> clobber errno (e.g., they pass errno to error_setg_errno() and return
> -errno afterwards). Instead of trying to find and fix all of these
> constructs, just make sure error_setg() and error_setg_errno() indeed do
> not clobber errno.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  util/error.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan