[PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save

[PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save

[Olaf Hering]

The previous patch ("tools/libxc: pass errno to callers of
xc_domain_save") did not jump to the proper label if the domU is HVM.

Also make it clearer what "out" and "out_rc" are supposed to mean:
label "out" is for failure case, it expects errno to be set.
label "out_rc" is for success case, it preserves the initial errno for
the caller.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_domain_save.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 9f96bb7..5a50c5c 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -1837,7 +1837,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         
         /* HVM guests are done now */
         errno = 0;
-        goto out;
+        goto out_rc;
     }
 
     /* PV guests only from now on */
@@ -2037,13 +2037,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     }
 
     /* Success! */
-    rc = errno = 0;
+    errno = 0;
     goto out_rc;
 
  out:
-    rc = errno;
-    assert(rc);
+    assert(errno);
  out_rc:
+    rc = errno;
     completed = 1;
 
     if ( !rc && callbacks->postcopy )

Re: [PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save

[Ian Campbell]

On Thu, 2014-03-13 at 18:34 +0100, Olaf Hering wrote:
> The previous patch ("tools/libxc: pass errno to callers of
> xc_domain_save") did not jump to the proper label if the domU is HVM.
> 
> Also make it clearer what "out" and "out_rc" are supposed to mean:
> label "out" is for failure case, it expects errno to be set.
> label "out_rc" is for success case, it preserves the initial errno for
> the caller.

I wish you hadn't done this bit, since now I can't just throw the fix in
as I need to think about it harder, the changes are certainly
non-obvious.

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  tools/libxc/xc_domain_save.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 9f96bb7..5a50c5c 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -1837,7 +1837,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>          
>          /* HVM guests are done now */
>          errno = 0;
> -        goto out;
> +        goto out_rc;
>      }
>  
>      /* PV guests only from now on */
> @@ -2037,13 +2037,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>      }
>  
>      /* Success! */
> -    rc = errno = 0;
> +    errno = 0;
>      goto out_rc;
>  
>   out:
> -    rc = errno;
> -    assert(rc);
> +    assert(errno);
>   out_rc:
> +    rc = errno;
>      completed = 1;
>  
>      if ( !rc && callbacks->postcopy )
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save

[Andrew Cooper]

On 13/03/14 17:34, Olaf Hering wrote:
> The previous patch ("tools/libxc: pass errno to callers of
> xc_domain_save") did not jump to the proper label if the domU is HVM.
>
> Also make it clearer what "out" and "out_rc" are supposed to mean:
> label "out" is for failure case, it expects errno to be set.
> label "out_rc" is for success case, it preserves the initial errno for
> the caller.

Surely "out_rc" should be renamed to "success" then?

~Andrew

>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  tools/libxc/xc_domain_save.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 9f96bb7..5a50c5c 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -1837,7 +1837,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>          
>          /* HVM guests are done now */
>          errno = 0;
> -        goto out;
> +        goto out_rc;
>      }
>  
>      /* PV guests only from now on */
> @@ -2037,13 +2037,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>      }
>  
>      /* Success! */
> -    rc = errno = 0;
> +    errno = 0;
>      goto out_rc;
>  
>   out:
> -    rc = errno;
> -    assert(rc);
> +    assert(errno);
>   out_rc:
> +    rc = errno;
>      completed = 1;
>  
>      if ( !rc && callbacks->postcopy )
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save

[Ian Jackson]

Olaf Hering writes ("[PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save"):
> The previous patch ("tools/libxc: pass errno to callers of
> xc_domain_save") did not jump to the proper label if the domU is HVM.
> 
> Also make it clearer what "out" and "out_rc" are supposed to mean:
> label "out" is for failure case, it expects errno to be set.
> label "out_rc" is for success case, it preserves the initial errno for
> the caller.

I agree with Ian that this is very confusing.  How about this instead ?

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 9f96bb7..71f9b59 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -1836,8 +1836,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         }
         
         /* HVM guests are done now */
-        errno = 0;
-        goto out;
+        goto success;
     }
 
     /* PV guests only from now on */
@@ -2037,6 +2036,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     }
 
     /* Success! */
+ success:
     rc = errno = 0;
     goto out_rc;

Re: [PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save

[Olaf Hering]

On Thu, Mar 13, Ian Jackson wrote:

> Olaf Hering writes ("[PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save"):
> > The previous patch ("tools/libxc: pass errno to callers of
> > xc_domain_save") did not jump to the proper label if the domU is HVM.
> > 
> > Also make it clearer what "out" and "out_rc" are supposed to mean:
> > label "out" is for failure case, it expects errno to be set.
> > label "out_rc" is for success case, it preserves the initial errno for
> > the caller.
> 
> I agree with Ian that this is very confusing.  How about this instead ?

That would fix the current bug, yes.

Olaf

Re: [PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH] tools/libxc: fix errno handling for HVM in xc_domain_save"):
> On Thu, Mar 13, Ian Jackson wrote:
> > I agree with Ian that this is very confusing.  How about this instead ?
> 
> That would fix the current bug, yes.

Thanks for the review.  I have pushed this, in the form below.

Ian.

commit 29f0712bc008a9d6e611785424f797cfc185c1d3
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Thu Mar 13 17:59:49 2014 +0000

    tools/libxc: fix errno handling for HVM in xc_domain_save
    
    The previous patch (dda0b77d "tools/libxc: pass errno to callers of
    xc_domain_save") did not jump to the proper label if the domU is HVM.
    
    Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
    Reviewed-by: Olaf Hering <olaf@aepfle.de>

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 9f96bb7..71f9b59 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -1836,8 +1836,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         }
         
         /* HVM guests are done now */
-        errno = 0;
-        goto out;
+        goto success;
     }
 
     /* PV guests only from now on */
@@ -2037,6 +2036,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     }
 
     /* Success! */
+ success:
     rc = errno = 0;
     goto out_rc;