[Xen-devel Patch 0/2] Prevent xc_domain_restore() returning success despite errors

[Xen-devel Patch 0/2] Prevent xc_domain_restore() returning success despite errors

[Andrew Cooper]

When looking through the XenServer patch queue, I noticed two bugfixes which
really should make their way upstream.

Both of these errors have been discovered by xc_domain_restore() returning
success after suffering a fatal error during migration, leading to the
toolstack believing that the VM migrated successfully.

Regarding 4.4:

I know this is quite late in the 4.4 cycle, and I apologise for not
noticing and upstreaming earlier, but I believe that these two fixes should
be considered for inclusion into 4.4.

They are both real errors found by XenRT causing mismanagement of migrated
domains. Both patches are small, concise, and (I believe) well explained.

The the use of various '*rc' variables can be easily viewed using `grep "rc "
xc_domain_restore.c`.  The risks are that I have made a mistake which could
result in further migration errors.  However, that risk is mitigated by
functionally-similar fixes being present in XenServer, and hopefully from the
obvious nature of the patches.  Futhermore, the changes themselves are for
error paths.

On the other hand, if they are deemed too risky (or buggy given review), it
will not be the end of the world if not included, although I hope that is not
the case.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>

-- 
1.7.10.4

[Patch 1/2] tools/libxc: goto correct label on error paths

[Andrew Cooper]

Both of these "goto finish;" statements are actually errors, and need to "goto
out;" instead, which will correctly destroy the domain and return an error,
rather than trying to finish the migration (and in at least one scenario,
return success).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/libxc/xc_domain_restore.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index ca2fb51..5ba47d7 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -1778,14 +1778,14 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
 
     if ( pagebuf_get(xch, ctx, &pagebuf, io_fd, dom) ) {
         PERROR("error when buffering batch, finishing");
-        goto finish;
+        goto out;
     }
     memset(&tmptail, 0, sizeof(tmptail));
     tmptail.ishvm = hvm;
     if ( buffer_tail(xch, ctx, &tmptail, io_fd, max_vcpu_id, vcpumap,
                      ext_vcpucontext, vcpuextstate, vcpuextstate_size) < 0 ) {
         ERROR ("error buffering image tail, finishing");
-        goto finish;
+        goto out;
     }
     tailbuf_free(&tailbuf);
     memcpy(&tailbuf, &tmptail, sizeof(tailbuf));
-- 
1.7.10.4

Re: [Patch 1/2] tools/libxc: goto correct label on error paths

[George Dunlap]

On 01/27/2014 04:25 PM, Andrew Cooper wrote:
> Both of these "goto finish;" statements are actually errors, and need to "goto
> out;" instead, which will correctly destroy the domain and return an error,
> rather than trying to finish the migration (and in at least one scenario,
> return success).
>
> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
> CC: Ian Campbell<Ian.Campbell@citrix.com>
> CC: Ian Jackson<Ian.Jackson@eu.citrix.com>
> CC: George Dunlap<george.dunlap@eu.citrix.com>

Right -- I can't imagine any goodness coming from jumping to "finish" in 
those cases...

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
>   tools/libxc/xc_domain_restore.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index ca2fb51..5ba47d7 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -1778,14 +1778,14 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>   
>       if ( pagebuf_get(xch, ctx, &pagebuf, io_fd, dom) ) {
>           PERROR("error when buffering batch, finishing");
> -        goto finish;
> +        goto out;
>       }
>       memset(&tmptail, 0, sizeof(tmptail));
>       tmptail.ishvm = hvm;
>       if ( buffer_tail(xch, ctx, &tmptail, io_fd, max_vcpu_id, vcpumap,
>                        ext_vcpucontext, vcpuextstate, vcpuextstate_size) < 0 ) {
>           ERROR ("error buffering image tail, finishing");
> -        goto finish;
> +        goto out;
>       }
>       tailbuf_free(&tailbuf);
>       memcpy(&tailbuf, &tmptail, sizeof(tailbuf));

Re: [Patch 1/2] tools/libxc: goto correct label on error paths

[Ian Campbell]

On Tue, 2014-01-28 at 11:41 +0000, George Dunlap wrote:
> On 01/27/2014 04:25 PM, Andrew Cooper wrote:
> > Both of these "goto finish;" statements are actually errors, and need to "goto
> > out;" instead, which will correctly destroy the domain and return an error,
> > rather than trying to finish the migration (and in at least one scenario,
> > return success).
> >
> > Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
> > CC: Ian Campbell<Ian.Campbell@citrix.com>
> > CC: Ian Jackson<Ian.Jackson@eu.citrix.com>
> > CC: George Dunlap<george.dunlap@eu.citrix.com>
> 
> Right -- I can't imagine any goodness coming from jumping to "finish" in 
> those cases...
> 
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked + applied, thanks.

[Patch 2/2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Andrew Cooper]

The variable 'rc' is set to 1 at the top of xc_domain_restore, and for the
most part is left alone until success, at which point it is set to 0.

There is a separate 'frc' which for the most part is used to check function
calls, keeping errors separate from 'rc'.

For a toolstack which sets callbacks->toolstack_restore(), and the function
returns 0, any subsequent error will end up with code flow going to "out;",
resulting in the migration being declared a success.

For consistency, update the callsites of xc_dom_gnttab{,_hvm}_seed() to use
'frc', even though their use of 'rc' is currently safe.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>

---

Regarding 4.4: If the two "for consistency" changes to
xc_dom_gnttab{,_hvm}_seed() are considered too risky, they can be dropped
without affecting the bugfix nature of the patch, but I would argue that
leaving some examples of "rc = function_call()" leaves a bad precident which
is likely to lead to similar bugs in the future.
---
 tools/libxc/xc_domain_restore.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 5ba47d7..817054d 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -2240,9 +2240,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
         memcpy(ctx->live_p2m, ctx->p2m, dinfo->p2m_size * sizeof(xen_pfn_t));
     munmap(ctx->live_p2m, P2M_FL_ENTRIES * PAGE_SIZE);
 
-    rc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
-                            console_domid, store_domid);
-    if (rc != 0)
+    frc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
+                             console_domid, store_domid);
+    if (frc != 0)
     {
         ERROR("error seeding grant table");
         goto out;
@@ -2257,16 +2257,15 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
     {
         if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
         {
-            rc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
-                        callbacks->data);
+            frc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
+                                               callbacks->data);
             free(tdata.data);
-            if ( rc < 0 )
+            if ( frc < 0 )
             {
                 PERROR("error calling toolstack_restore");
                 goto out;
             }
         } else {
-            rc = -1;
             ERROR("toolstack data available but no callback provided\n");
             free(tdata.data);
             goto out;
@@ -2326,9 +2325,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
         goto out;
     }
 
-    rc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
-                                console_domid, store_domid);
-    if (rc != 0)
+    frc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
+                                 console_domid, store_domid);
+    if (frc != 0)
     {
         ERROR("error seeding grant table");
         goto out;
-- 
1.7.10.4

Re: [Patch 2/2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Andrew Cooper]

On 27/01/14 16:25, Andrew Cooper wrote:
> The variable 'rc' is set to 1 at the top of xc_domain_restore, and for the
> most part is left alone until success, at which point it is set to 0.
>
> There is a separate 'frc' which for the most part is used to check function
> calls, keeping errors separate from 'rc'.
>
> For a toolstack which sets callbacks->toolstack_restore(), and the function
> returns 0, any subsequent error will end up with code flow going to "out;",
> resulting in the migration being declared a success.
>
> For consistency, update the callsites of xc_dom_gnttab{,_hvm}_seed() to use
> 'frc', even though their use of 'rc' is currently safe.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>

Ping?

>
> ---
>
> Regarding 4.4: If the two "for consistency" changes to
> xc_dom_gnttab{,_hvm}_seed() are considered too risky, they can be dropped
> without affecting the bugfix nature of the patch, but I would argue that
> leaving some examples of "rc = function_call()" leaves a bad precident which
> is likely to lead to similar bugs in the future.
> ---
>  tools/libxc/xc_domain_restore.c |   19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index 5ba47d7..817054d 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -2240,9 +2240,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>          memcpy(ctx->live_p2m, ctx->p2m, dinfo->p2m_size * sizeof(xen_pfn_t));
>      munmap(ctx->live_p2m, P2M_FL_ENTRIES * PAGE_SIZE);
>  
> -    rc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
> -                            console_domid, store_domid);
> -    if (rc != 0)
> +    frc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
> +                             console_domid, store_domid);
> +    if (frc != 0)
>      {
>          ERROR("error seeding grant table");
>          goto out;
> @@ -2257,16 +2257,15 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>      {
>          if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
>          {
> -            rc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
> -                        callbacks->data);
> +            frc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
> +                                               callbacks->data);
>              free(tdata.data);
> -            if ( rc < 0 )
> +            if ( frc < 0 )
>              {
>                  PERROR("error calling toolstack_restore");
>                  goto out;
>              }
>          } else {
> -            rc = -1;
>              ERROR("toolstack data available but no callback provided\n");
>              free(tdata.data);
>              goto out;
> @@ -2326,9 +2325,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>          goto out;
>      }
>  
> -    rc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
> -                                console_domid, store_domid);
> -    if (rc != 0)
> +    frc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
> +                                 console_domid, store_domid);
> +    if (frc != 0)
>      {
>          ERROR("error seeding grant table");
>          goto out;

Re: [Patch 2/2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Ian Campbell]

On Tue, 2014-02-04 at 17:16 +0000, Andrew Cooper wrote:
> >                  goto out;
> >              }
> >          } else {
> > -            rc = -1;

Mostly looks good but I'm not sure about this change 

We get here on input error (toolstack data available but no callback
provided) which is neither migration success nor failure, it's a bug in
the caller. So arguably returning a separate failure from
success/unsuccess makes sense.

I'd have thought it ought to set errno (too EINVAL perhaps) too, but
lets not mess with that now.


Ian.

Re: [Patch 2/2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Andrew Cooper]

On 04/02/14 17:22, Ian Campbell wrote:
> On Tue, 2014-02-04 at 17:16 +0000, Andrew Cooper wrote:
>>>                  goto out;
>>>              }
>>>          } else {
>>> -            rc = -1;
> Mostly looks good but I'm not sure about this change 
>
> We get here on input error (toolstack data available but no callback
> provided) which is neither migration success nor failure, it's a bug in
> the caller. So arguably returning a separate failure from
> success/unsuccess makes sense.
>
> I'd have thought it ought to set errno (too EINVAL perhaps) too, but
> lets not mess with that now.
>
>
> Ian.
>

Hilariously, it turns out that xc_domain_restore() is specified to
return 0 on success and -1 on failure.  From what I can tell, this is
the sole action which would cause xc_domain_restore() to return anything
other than 0 or 1.

I think fixing this should fall into the bucket of "sanitisation of
libxc error paths".

~Andrew

Re: [Patch 2/2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Ian Campbell]

On Tue, 2014-02-04 at 17:26 +0000, Andrew Cooper wrote:
> On 04/02/14 17:22, Ian Campbell wrote:
> > On Tue, 2014-02-04 at 17:16 +0000, Andrew Cooper wrote:
> >>>                  goto out;
> >>>              }
> >>>          } else {
> >>> -            rc = -1;
> > Mostly looks good but I'm not sure about this change 
> >
> > We get here on input error (toolstack data available but no callback
> > provided) which is neither migration success nor failure, it's a bug in
> > the caller. So arguably returning a separate failure from
> > success/unsuccess makes sense.
> >
> > I'd have thought it ought to set errno (too EINVAL perhaps) too, but
> > lets not mess with that now.
> >
> >
> > Ian.
> >
> 
> Hilariously, it turns out that xc_domain_restore() is specified to
> return 0 on success and -1 on failure.  From what I can tell, this is
> the sole action which would cause xc_domain_restore() to return anything
> other than 0 or 1.
> 
> I think fixing this should fall into the bucket of "sanitisation of
> libxc error paths".

Right, so please leave the rc = -1 alone.

Ian.

[PATCH v2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Andrew Cooper]

The variable 'rc' is set to 1 at the top of xc_domain_restore, and for the
most part is left alone until success, at which point it is set to 0.

There is a separate 'frc' which for the most part is used to check function
calls, keeping errors separate from 'rc'.

For a toolstack which sets callbacks->toolstack_restore(), and the function
returns 0, any subsequent error will end up with code flow going to "out;",
resulting in the migration being declared a success.

For consistency, update the callsites of xc_dom_gnttab{,_hvm}_seed() to use
'frc', even though their use of 'rc' is currently safe.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>

---

Changes in v2:
 * Dont drop rc = -1 from toolstack_restore().

Regarding 4.4: If the two "for consistency" changes to
xc_dom_gnttab{,_hvm}_seed() are considered too risky, they can be dropped
without affecting the bugfix nature of the patch, but I would argue that
leaving some examples of "rc = function_call()" leaves a bad precident which
is likely to lead to similar bugs in the future.
---
 tools/libxc/xc_domain_restore.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 5ba47d7..1f6ce50 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -2240,9 +2240,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
         memcpy(ctx->live_p2m, ctx->p2m, dinfo->p2m_size * sizeof(xen_pfn_t));
     munmap(ctx->live_p2m, P2M_FL_ENTRIES * PAGE_SIZE);
 
-    rc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
-                            console_domid, store_domid);
-    if (rc != 0)
+    frc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
+                             console_domid, store_domid);
+    if (frc != 0)
     {
         ERROR("error seeding grant table");
         goto out;
@@ -2257,10 +2257,10 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
     {
         if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
         {
-            rc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
-                        callbacks->data);
+            frc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
+                                               callbacks->data);
             free(tdata.data);
-            if ( rc < 0 )
+            if ( frc < 0 )
             {
                 PERROR("error calling toolstack_restore");
                 goto out;
@@ -2326,9 +2326,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
         goto out;
     }
 
-    rc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
-                                console_domid, store_domid);
-    if (rc != 0)
+    frc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
+                                 console_domid, store_domid);
+    if (frc != 0)
     {
         ERROR("error seeding grant table");
         goto out;
-- 
1.7.10.4

Re: [PATCH v2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Ian Campbell]

On Tue, 2014-02-04 at 18:01 +0000, Andrew Cooper wrote:
> The variable 'rc' is set to 1 at the top of xc_domain_restore, and for the
> most part is left alone until success, at which point it is set to 0.
> 
> There is a separate 'frc' which for the most part is used to check function
> calls, keeping errors separate from 'rc'.
> 
> For a toolstack which sets callbacks->toolstack_restore(), and the function
> returns 0, any subsequent error will end up with code flow going to "out;",
> resulting in the migration being declared a success.
> 
> For consistency, update the callsites of xc_dom_gnttab{,_hvm}_seed() to use
> 'frc', even though their use of 'rc' is currently safe.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> 
> ---
> 
> Changes in v2:
>  * Dont drop rc = -1 from toolstack_restore().
> 
> Regarding 4.4: If the two "for consistency" changes to
> xc_dom_gnttab{,_hvm}_seed() are considered too risky, they can be dropped
> without affecting the bugfix nature of the patch, but I would argue that
> leaving some examples of "rc = function_call()" leaves a bad precident which
> is likely to lead to similar bugs in the future.
> ---
>  tools/libxc/xc_domain_restore.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index 5ba47d7..1f6ce50 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -2240,9 +2240,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>          memcpy(ctx->live_p2m, ctx->p2m, dinfo->p2m_size * sizeof(xen_pfn_t));
>      munmap(ctx->live_p2m, P2M_FL_ENTRIES * PAGE_SIZE);
>  
> -    rc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
> -                            console_domid, store_domid);
> -    if (rc != 0)
> +    frc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
> +                             console_domid, store_domid);
> +    if (frc != 0)
>      {
>          ERROR("error seeding grant table");
>          goto out;
> @@ -2257,10 +2257,10 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>      {
>          if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
>          {
> -            rc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
> -                        callbacks->data);
> +            frc = callbacks->toolstack_restore(dom, tdata.data, tdata.len,
> +                                               callbacks->data);
>              free(tdata.data);
> -            if ( rc < 0 )
> +            if ( frc < 0 )
>              {
>                  PERROR("error calling toolstack_restore");
>                  goto out;
> @@ -2326,9 +2326,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>          goto out;
>      }
>  
> -    rc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
> -                                console_domid, store_domid);
> -    if (rc != 0)
> +    frc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
> +                                 console_domid, store_domid);
> +    if (frc != 0)
>      {
>          ERROR("error seeding grant table");
>          goto out;

Re: [PATCH v2] tools/libxc: Prevent erroneous success from xc_domain_restore

[George Dunlap]

On 02/04/2014 06:01 PM, Andrew Cooper wrote:
> The variable 'rc' is set to 1 at the top of xc_domain_restore, and for the
> most part is left alone until success, at which point it is set to 0.
>
> There is a separate 'frc' which for the most part is used to check function
> calls, keeping errors separate from 'rc'.
>
> For a toolstack which sets callbacks->toolstack_restore(), and the function
> returns 0, any subsequent error will end up with code flow going to "out;",
> resulting in the migration being declared a success.
>
> For consistency, update the callsites of xc_dom_gnttab{,_hvm}_seed() to use
> 'frc', even though their use of 'rc' is currently safe.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
>
> ---
>
> Changes in v2:
>   * Dont drop rc = -1 from toolstack_restore().
>
> Regarding 4.4: If the two "for consistency" changes to
> xc_dom_gnttab{,_hvm}_seed() are considered too risky, they can be dropped
> without affecting the bugfix nature of the patch, but I would argue that
> leaving some examples of "rc = function_call()" leaves a bad precident which
> is likely to lead to similar bugs in the future.

Yes, these are all pretty clear bug fixes.

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH v2] tools/libxc: Prevent erroneous success from xc_domain_restore

[Ian Campbell]

On Wed, 2014-02-05 at 14:55 +0000, George Dunlap wrote:
> On 02/04/2014 06:01 PM, Andrew Cooper wrote:
> > The variable 'rc' is set to 1 at the top of xc_domain_restore, and for the
> > most part is left alone until success, at which point it is set to 0.
> >
> > There is a separate 'frc' which for the most part is used to check function
> > calls, keeping errors separate from 'rc'.
> >
> > For a toolstack which sets callbacks->toolstack_restore(), and the function
> > returns 0, any subsequent error will end up with code flow going to "out;",
> > resulting in the migration being declared a success.
> >
> > For consistency, update the callsites of xc_dom_gnttab{,_hvm}_seed() to use
> > 'frc', even though their use of 'rc' is currently safe.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > CC: Ian Campbell <Ian.Campbell@citrix.com>
> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: George Dunlap <george.dunlap@eu.citrix.com>
> >
> > ---
> >
> > Changes in v2:
> >   * Dont drop rc = -1 from toolstack_restore().
> >
> > Regarding 4.4: If the two "for consistency" changes to
> > xc_dom_gnttab{,_hvm}_seed() are considered too risky, they can be dropped
> > without affecting the bugfix nature of the patch, but I would argue that
> > leaving some examples of "rc = function_call()" leaves a bad precident which
> > is likely to lead to similar bugs in the future.
> 
> Yes, these are all pretty clear bug fixes.
> 
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Applied.