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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>

Acked-by: Ian Campbell <Ian.Campbell@xxxxxxxxxx>

> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> ---
> 
> 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;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel