Re: [PATCH - policyrep] add objpool to libsepol

[James Antill <jantill@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3316 bytes --]

On Thu, 2007-04-19 at 11:35 -0400, Karl MacMillan wrote:
> On Thu, 2007-04-19 at 00:47 -0400, James Antill wrote:
> > On Wed, 2007-04-18 at 21:57 -0400, Karl MacMillan wrote:
> > > +int sepol_objpool_free(struct sepol_handle *h, struct sepol_objpool *s)
> > 
> >  If this is going to be used a lot having only a single "destroy"
> > function that also silently unref's everything it's managing seems
> > "bad".
> 
> I'm only intending to use this in the new policyrep where the lifetime
> of the object pool will be tied to the lifetime of the policy tree. I
> could add a destroy function that just destroys the pool though. I'd
> likely need to add a way to iterate over all of the objects in the pool
> to make this generally useful.

 So, sepol_objpool_del() will never be called? If so it seems better to
have it work like an APR memory pool or obstack etc. where you allocate
a bunch of small objects but only remove the entire thing once.

> > > +	if (ret == SEPOL_OK) {
> > 
> >  This doesn't copy "*obj" and so assumes s->obj_free is used to free the
> > input.
> 
> Yes.
> 
> > > +		return 0;
> > > +	} else if (ret == SEPOL_EEXIST) {
> > > +		free(refcount);
> > > +		s->obj_free(*obj);
> > > +		*obj = cur->key;
> > > +		refcount = cur->datum;
> > > +		*refcount += 1;
> > > +		return 0;
> > 
> >  This also assumes s->obj_free is the correct thing to use for the
> > input.
> 
> Yes - that is intentional to avoid additional copying. Are you pointing
> out a problem, or just suggesting the constraint needs documentation?

 I guess I just wonder why have the s->obj_free member at all, esp.
given the examples used with strdup() ... I kind of assumed you expected
input from strdup(), given the examples, and so was confused about the
non-useage of free().
 It also just seems wrong to have the member free function used for
input ... but if you have a use case feel free to ignore me :).

> > > +	} else {
> > > +		free(refcount);
> > > +		return ret;
> > > +	}
> > > +}
> > [...]
> > > +int sepol_objpool_del(struct sepol_handle *h, struct sepol_objpool *s, void *obj)
> > > +{
> > > +	int ret;
> > > +	unsigned int *refcount;
> > > +	
> > > +	refcount = hashtab_search(s->objs, obj);
> > > +	if (refcount == NULL)
> > > +		return SEPOL_ENOENT;
> > > +	
> > > +	(*refcount)--;
> > > +	if (*refcount == 0) {
> > > +		ret = hashtab_remove(s->objs, obj,
> > > +				     sepol_objpool_hashtab_free, s);
> > > +		if (ret != SEPOL_OK)
> > > +			return ret;
> > > +	}
> > 
> >  The only thing hashtab_remove() returns, apart from _OK, is _ENOENT ...
> > which would mean it could be unref'd or not ... if hashtab_remove()
> > could return it in this codepath.
> 
> Not certain what you mean. Generally, these APIs aren't thread safe so I
> assume that the remove will succeed here. The error checking is for
> future changes in the hashtab implementation.

 Sorry, bad English on my part. My point was that I don't see how
hashtab_remove() could ever return in error here (as you've already made
sure it is there) ... and even if it ever did return in error at some
future point, you are leaking a reference count (*refcount == 0, but
there is still one reference in the hashtab).

-- 
James Antill <jantill@redhat.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]