Re: [PATCH - policyrep] add objpool to libsepol

[James Antill <jantill@redhat.com>]

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

On Wed, 2007-04-18 at 21:57 -0400, Karl MacMillan wrote:
> Add a reference counted object pool data structure to libsepol. Object
> pools allow the storage of reference counted pools of objects. This
> allows, for example, for many data structures to reference a single copy
> of an object (e.g., a string).

 As a general observation this makes me want to cry for the silent death
of a common C string ADT. But everyone has to reinvent their own so...

> +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".
 One obvious problem this causes is that if sepol_objpool_free() returns
in error you don't know if it's from the iter create (everything fine,
try again) or from the sepol_iter_free (everything free'd already ...
you are totally screwed). Luckily sepol_iter_free can't ever do that.

> +int sepol_objpool_add(struct sepol_handle *h, struct sepol_objpool *s, void **obj)

 One minor point here is that while (char *) can change to (void *),
(char **) cannot change to (void **), legally. see:

http://c-faq.com/ptrs/genericpp.html

...having a real string ADT would do the right thing (cough) ... ok I'll
shutup now.

> +{
> +	int ret;
> +	hashtab_ptr_t cur;
> +	unsigned int *refcount;
> +
> +	refcount = malloc(sizeof(unsigned int));
> +	if (refcount == NULL)
> +		return SEPOL_ENOMEM;
> +	*refcount = 1;
> +
> +	ret = hashtab_insert2(s->objs, *obj, refcount, &cur);

 hashtab_insert2() seems like a hack based on the fact you can't use the
result of hashtab_search().

> +	if (ret == SEPOL_OK) {

 This doesn't copy "*obj" and so assumes s->obj_free is used to free the
input.

> +		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.

> +	} 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.
 Also if you fixed the hashtab_search followed by hashtab_insert
problem, then this codepath could also benefit in the same way (or maybe
just have hashtab_remove2 ;).

-- 
James Antill <jantill@redhat.com>

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