From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v2 08/13] libxc: Check xc_domain_maximum_gpfn for negative return values Date: Thu, 19 Mar 2015 14:54:16 -0400 Message-ID: <20150319185416.GC21217@x230.dumpdata.com> References: <1426724659-23999-1-git-send-email-konrad.wilk@oracle.com> <1426724659-23999-9-git-send-email-konrad.wilk@oracle.com> <1426783678.21742.75.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YYfaM-0005tP-6g for xen-devel@lists.xenproject.org; Thu, 19 Mar 2015 18:54:26 +0000 Content-Disposition: inline In-Reply-To: <1426783678.21742.75.camel@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: Ian Campbell Cc: xen-devel@lists.xenproject.org, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Thu, Mar 19, 2015 at 04:47:58PM +0000, Ian Campbell wrote: > On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote: > > Instead of assuming everything is always OK. We stash > > the gpfns value as an parameter. > > > > Signed-off-by: Konrad Rzeszutek Wilk > > --- > > tools/libxc/xc_core_arm.c | 17 ++++++++++++++--- > > tools/libxc/xc_core_x86.c | 24 ++++++++++++++++++++---- > > tools/libxc/xc_domain_save.c | 8 +++++++- > > 3 files changed, 41 insertions(+), 8 deletions(-) > > > > diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c > > index 16508e7..26cec04 100644 > > --- a/tools/libxc/xc_core_arm.c > > +++ b/tools/libxc/xc_core_arm.c > > @@ -31,9 +31,16 @@ xc_core_arch_gpfn_may_present(struct xc_core_arch_context *arch_ctxt, > > } > > > > > > -static int nr_gpfns(xc_interface *xch, domid_t domid) > > +static int nr_gpfns(xc_interface *xch, domid_t domid, unsigned long *gpfns) > > You didn't fancy merging the two versions of this then ;-) I was not sure where you would want to put them. xc_private looks like the best place, but perhaps it should be in an new file? > > diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c > > index d8846f1..02377e8 100644 > > --- a/tools/libxc/xc_core_x86.c > > +++ b/tools/libxc/xc_core_x86.c > > > @@ -88,7 +99,12 @@ xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc > > int err; > > int i; > > > > - dinfo->p2m_size = nr_gpfns(xch, info->domid); > > + err = nr_gpfns(xch, info->domid, &dinfo->p2m_size); > > Please could you avoid reusing err here, the reason is that it's sole > use now is to save errno over the cleanup path, whereas here it looks > like it is going to be used for something but it isn't. > > if ( nr_gpfns(...) < 0 ) > > is ok per the Xen coding style if you don't actually need the return > code. > > Or > > ret = nr_gpfns() > if ( ret < 0 ) > error, goto out > > ret = -1; > .. the rest > > would be ok too I guess. (coding style here allows > if ( (ret = nr_gpfns(...)) < 0 ) > too FWIW). > > > + if ( err < 0 ) > > + { > > + ERROR("nr_gpfns returns errno: %d.", errno); > > + goto out; > > + } > > if ( dinfo->p2m_size < info->nr_pages ) > > { > > ERROR("p2m_size < nr_pages -1 (%lx < %lx", dinfo->p2m_size, info->nr_pages - 1); > > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c > > index 254fdb3..6346c12 100644 > > --- a/tools/libxc/xc_domain_save.c > > +++ b/tools/libxc/xc_domain_save.c > > @@ -939,7 +939,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter > > } > > > > /* Get the size of the P2M table */ > > - dinfo->p2m_size = xc_domain_maximum_gpfn(xch, dom) + 1; > > + rc = xc_domain_maximum_gpfn(xch, dom); > > + if ( rc < 0 ) > > + { > > + ERROR("Could not get maximum GPFN!"); > > + goto out; > > + } > > + dinfo->p2m_size = rc + 1; > > Shame this can't use the same helper as the others. But if we do stick that 'nr_gpfns' in xc_private.c it could! > > Ian. >