Re: [Xen-devel] [PATCH v2 08/13] libxc: Check xc_domain_maximum_gpfn for negative return values

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-03-20 at 10:49 -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 20, 2015 at 10:34:34AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Fri, Mar 20, 2015 at 09:48:08AM +0000, Ian Campbell wrote:
> > > On Thu, 2015-03-19 at 14:54 -0400, Konrad Rzeszutek Wilk wrote:
> > > > 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 <konrad.wilk@xxxxxxxxxx>
> > > > > > ---
> > > > > >  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?
> > > 
> > > I also suggested just changing the interface of xc_domain_maximum_gpfn,
> > > in which case it can stay in xc_domain.c. TBH there seems little point
> > > in xc_domain_maximum_gpfn if all callers are using a wrapper, so I think
> > > I'd advocate this approach.
> > 
> > Duh, that would be much simpler. Let me respin a patch for that.
> 
> Running through testing with it.
> 
> All of them are 
> 
>  git://xenbits.xen.org/people/konradwilk/xen.git xc_cleanup.v4
> 
> 
> From 319763b12a8c44722f5f170476e0d2afe03408c2 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date: Fri, 13 Mar 2015 14:57:44 -0400
> Subject: [PATCH] libxc: Check xc_domain_maximum_gpfn for negative return
>  values
> 
> Instead of assuming everything is always OK. We stash
> the gpfns value as an parameter. Since we use it in three
> of places we might as well update xc_domain_maximum_gpfn
> to do the right thing.
> 
> Suggested-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Looks good, only one minor comment:
[...]

> -    *gpfn = (xen_pfn_t)rc + 1;

Perhaps the new parameter to xc_domain_maximum_gpfn should be a
xen_pfn_t?



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