From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: Possible bug in tools/libxl/libxl.c -- Variable passed by reference not set in one possible case Date: Fri, 10 Oct 2014 10:30:06 +0100 Message-ID: <1412933406.27111.4.camel@citrix.com> References: <20141010090611.GB4880@zion.uk.xensource.com> <1412932635.10650.22.camel@citrix.com> <20141010092440.GB6178@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141010092440.GB6178@zion.uk.xensource.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: Wei Liu Cc: xen-devel@lists.xen.org, ayush ruia List-Id: xen-devel@lists.xenproject.org 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.