From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 16/29] libxl: ao: Provide manip_refcnt Date: Wed, 1 Apr 2015 10:34:26 +0100 Message-ID: <1427880866.2115.252.camel@citrix.com> References: <1423599016-32639-1-git-send-email-ian.jackson@eu.citrix.com> <1423599016-32639-17-git-send-email-ian.jackson@eu.citrix.com> <1427196722.21742.354.camel@citrix.com> <21786.58929.766833.923254@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21786.58929.766833.923254@mariner.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: Ian Jackson Cc: xen-devel@lists.xensource.com, Euan Harris List-Id: xen-devel@lists.xenproject.org On Tue, 2015-03-31 at 19:23 +0100, Ian Jackson wrote: > Ian Campbell writes ("Re: [Xen-devel] [PATCH 16/29] libxl: ao: Provide manip_refcnt"): > > On Tue, 2015-02-10 at 20:10 +0000, Ian Jackson wrote: > > > +/* > > > + * A "manip" is a libxl public function manipulating this ao, which > > > + * has a pointer to it. We have to not destroy it while that's the > > > + * case, obviously. > > > > It might be nice to the reader to make a reference to the "An ao and its > > gc may be accessed only with the ctx lock held." sentence in the overall > > ao docs? > > This seems to have been documented already in May 2012 in exactly > those terms, at the top of struct libxl__ao, in 738b3d442522. Am I > confused ? Right, my suggestions was to make a reference to that comment. One might expect ao__manip_enter to take care of locking for you as part of the "prepare this thing to be manipulated". We don't seem to have an ASSERT_CTX_LOCKED construct, or I would suggest using it in cases like these. > I have added a comment about locking in the manip functions' doc > comment. > > > [...] > > > + libxl__ao__destroy(ctx,ao); > > > > Nit: missing space after ",". > > Fixed. > > > But other than those and including if you disagree about extending the > > comment, since it's not a big deal: > > Acked-by: Ian Campbell This stands. Ian.