Re: Possible bug in tools/libxl/libxl.c -- Variable passed by reference not set in one possible case

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-10-10 at 10:24 +0100, Wei Liu wrote:
> On Fri, Oct 10, 2014 at 10:17:15AM +0100, Ian Campbell wrote:
> > On Fri, 2014-10-10 at 10:06 +0100, Wei Liu wrote:
> > > Please send plain text email in the future. Some (if not all) developers
> > > only have very shabby text editor to read / reply to your email. ;-)
> > > 
> > > On Thu, Oct 09, 2014 at 08:28:53PM -0500, ayush ruia wrote:
> > > > What I am trying to say is that shouldn't the code block highlighted in
> > > > yellow should come before the block marked in green. Then it would update
> > > > the value of *target_memkb and *max_memkb in all possible situations.
> > > > 
> > > 
> > > I don't think so. This function means "if there's no such values in
> > > xenstore then retrieve them from hypervisor and fill them in xenstore,
> > > optionally return those values to the caller".
> > > 
> > > So if those values already are present in xenstore this function doesn't
> > > need to do anything.
> > 
> > If I call this function and receive a return code of zero how can I tell
> > if the target_memkb pointer I passed has been initialised or not?
> > 
> > If all of target, staticmax and freememslack are already set the
> > function returns 0 without updating those pointers, so I don't think we
> > can tell.
> > 
> 
> The way the callers use it prevents the issue you described from
> happening -- they only call this function when they can't read those
> values from xenstore -- if those values are already in xenstore this
> function won't get called.

At least the callers in libxl__get_free_memory_slack and
libxl__get_memory_target look to be racy with someone else writing these
nodes to me.

libxl_set_memory_target is a bit unclear but the fact that it ends the
transaction right before it calls libxl__fill_dom0_memory_info seems
suspicious.

libxl__get_free_memory_slack is also separately suspicious because it
never uses the values anyway. I suppose just because
libxl__fill_dom0_memory_info doesn't tolerate NULL arguments like I
think it should.

Ian.

> 
> > It does seem like the 
> >  if (target && staticmax && freememslack) {
> >         rc = 0;
> >         goto out;
> >     }
> > belongs after the code which writes back the two variables.
> > 
> 
> I agree this is not nice and there's room for refactoring.
> 
> Wei.
> 
> > Ian.