From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Patch 2/2] tools/libxc: Prevent erroneous success from xc_domain_restore Date: Tue, 4 Feb 2014 17:16:59 +0000 Message-ID: <52F1208B.6040809@citrix.com> References: <1390839925-28088-1-git-send-email-andrew.cooper3@citrix.com> <1390839925-28088-3-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1390839925-28088-3-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: George Dunlap , Ian Jackson , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org 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 > CC: Ian Campbell > CC: Ian Jackson > CC: George Dunlap 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;