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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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;