[PATCH - policyrep] add objpool to libsepol

[PATCH - policyrep] add objpool to libsepol

[Karl MacMillan]

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

Signed-off-by: Karl MacMillan <kmacmillan@mentalrootkit.com>

diff -r 7e5ed01cd8ad libsepol/include/sepol/objpool.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/libsepol/include/sepol/objpool.h	Wed Apr 18 21:46:28 2007 -0400
@@ -0,0 +1,139 @@
+/* Author : Karl MacMillan <kmacmillan@mentalrootkit.com> */
+
+#ifndef __objpool_h__
+#define __objpool_h__
+
+#include <sepol/handle.h>
+
+/**
+ * \defgroup libsepol_objpool sepol_objpool: reference counted object pooling
+ * 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).
+ *
+ * Each object pool stores a single type of object. Hashing, comparison,
+ * and freeing functions must be provided by the user for the object
+ * to be stored. The objects are treated as generic void pointers by
+ * the object pool, and are not manipulated or interpreted in any way.
+ *
+ * The object pool manages the memory for objects. For example, consider
+ * the following example (error handling omitted for brevity):
+ *
+ * \code
+ * struct sepol_objpool *p;
+ * char *a = strdup("foo");
+ * char *b = strdup("foo");
+ * char *c = strdup("bar");
+ *
+ * // creation of object pool ommitted
+ * sepol_object_add(p, &a);
+ * sepol_object_add(p, &b);
+ * assert(a == b);
+ * // a and b now point to a single copy for the string "foo".
+ * // The copy originally pointed to by b has been destroyed.
+ * sepol_object_add(p, &c);
+ * sepol_object_del(p, c);
+ * // The object pool has destroyed its copy of the string "bar"
+ * // because the reference count became 0. The pointer c is no
+ * // longer valid.
+ * sepol_object_del(p, "foo");
+ * sepol_object_del(p, a);
+ * // deletion can use objects that compare as equal in addition
+ * // to pointers. The object pool is now empty.
+ * \endcode
+ */
+
+/* FIXME: These are the same as the hashing and key compare functions
+ * in policydb/hashtab.h - when hashtab.h is properly encapsulated and
+ * exported these should be merged.
+ */
+typedef unsigned int (*sepol_objpool_hash_t)(void *h, const void *key);
+typedef int (*sepol_objpool_cmp_t)(void *h, const void *key1, const void *key2);
+typedef void (*sepol_objpool_free_t)(void *obj);
+
+/**
+ * \ingroup libsepol_objpool
+ * \struct sepol_objpool
+ * The object pool data structure.
+ */
+struct sepol_objpool;
+
+/**
+ * \ingroup libsepol_objpool
+ * Create an object pool.
+ *
+ * Create an object pool with the appropriate functions for hashing,
+ * comparing, and freeing of the object type to be stored in the pool.
+ *
+ * @param h sepol handle (may be NULL)
+ * @param s pointer that will be set to the newly created object pool.
+ * @param hash pointer to a function to hash objects that will be managed
+ *  by the object pool. See sepol_symhash defined policydb/symtab.h for
+ *  an example hashing function.
+ * @param cmp pointer to a function to compare object that will be managed
+ *  by the object pool.
+ * @param obj_free pointer to a function to free the memory for objects
+ *  managed by the object pool.
+ * @param size size of the underlying hash table used to hold the objects.
+ *
+ * \retval SEPOL_OK success
+ * \retval SEPOL_ENOMEM out of memory
+ */
+extern int sepol_objpool_create(struct sepol_handle * h, struct sepol_objpool **s,
+				sepol_objpool_hash_t hash, sepol_objpool_cmp_t cmp,
+				sepol_objpool_free_t obj_free, unsigned int size);
+
+/**
+ * \ingroup libsepol_objpool
+ * Destroy an object pool. All of the memory associated with the object
+ * pool and all of the objects that it is managing is freed.
+ *
+ * @param h sepol handle (may be NULL)
+ * @param s object pool to destroy
+ *
+ * \retval SEPOL_OK success
+ * \retval SEPOL_ENOMEM out of memory
+ */
+extern int sepol_objpool_free(struct sepol_handle *h, struct sepol_objpool *s);
+
+/**
+ * \ingroup libsepol_objpool
+ * Add an object to the object pool. The pointer passed in
+ * will be set to point to the object pool copy of the object
+ * (the pointer may or may not change depending upon whether
+ * the object pool already contains a copy of the object). If
+ * the object pool already contains a copy of the passed in object
+ * the passed in copy will be freed and the reference count of
+ * the existing object will be incremented.
+ *
+ * @param h sepol handle (may be NULL)
+ * @param s object pool to which to add the object
+ * @param obj object to be added
+ *
+ * \retval SEPOL_OK success
+ * \retval SEPOL_ENOMEM out of memory
+ */
+extern int sepol_objpool_add(struct sepol_handle *h, struct sepol_objpool *s, void **obj);
+
+/**
+ * \ingroup libsepol_objpool
+ *
+ * Delete an object from the object pool. If other references to the
+ * object remain (from calls to sepol_objpool_add) the object will not
+ * be destroyed. However, the caller should *never* depend upon the
+ * object persisting beyond a call sepol_objpool_del. The object
+ * passed in does is not required to be a pointer returned by
+ * sepol_objpool_add; however, it is required to compare as equal to
+ * an existing object.
+ *
+ * @param h sepol handle (may be NULL)
+ * @param s object pool from which to delete object
+ * @param obj object to delete
+ *
+ * \retval SEPOL_OK success
+ * \retval SEPOL_ENOENT object does not exist in pool
+ * \retval SEPOL_ENOMEM out of memory
+ */
+extern int sepol_objpool_del(struct sepol_handle *h, struct sepol_objpool *s, void *obj);
+
+#endif
diff -r 7e5ed01cd8ad libsepol/src/objpool.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/libsepol/src/objpool.c	Wed Apr 18 21:46:45 2007 -0400
@@ -0,0 +1,135 @@
+/*
+ * Author : Karl MacMillan <kmacmillan@mentalrootkit.com>
+ *
+ * Copyright (C) 2007 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include <sepol/objpool.h>
+#include <sepol/policydb/hashtab.h>
+
+#include <stdlib.h>
+
+struct sepol_objpool
+{
+	hashtab_t objs;
+	sepol_objpool_free_t obj_free;
+};
+
+int sepol_objpool_create(struct sepol_handle *h, struct sepol_objpool **s,
+			 sepol_objpool_hash_t hash, sepol_objpool_cmp_t cmp,
+			 sepol_objpool_free_t obj_free, unsigned int size)
+{
+	*s = calloc(1, sizeof(struct sepol_objpool));
+	if (*s == NULL) {
+		return SEPOL_ENOMEM;
+	}
+
+	(*s)->obj_free = obj_free;
+
+	(*s)->objs = hashtab_create((hash_value_fnc_t)hash, (keycmp_fnc_t)cmp, size);
+	if ((*s)->objs == NULL) {
+		free(*s);
+		return SEPOL_ENOMEM;
+	}
+
+
+	return SEPOL_OK;
+}
+
+int sepol_objpool_free(struct sepol_handle *h, struct sepol_objpool *s)
+{
+	int ret;
+	struct sepol_iter *iter;
+	hashtab_ptr_t cur;
+
+	if (s == NULL)
+		return SEPOL_OK;
+
+	ret = sepol_iter_create(h, &iter);
+	if (ret < 0)
+		return ret;
+
+	ret = hashtab_iter(h, s->objs, iter);
+	sepol_foreach(h, ret, cur, iter) {
+		s->obj_free(cur->key);
+		free(cur->datum);
+	}
+	sepol_iter_free(h, iter);
+	if (ret != SEPOL_ITERSTOP) {
+		return ret;
+	}
+
+	hashtab_destroy(s->objs);
+	free(s);
+	
+	return SEPOL_OK;
+}
+
+
+int sepol_objpool_add(struct sepol_handle *h, struct sepol_objpool *s, void **obj)
+{
+	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);
+	if (ret == SEPOL_OK) {
+		return 0;
+	} else if (ret == SEPOL_EEXIST) {
+		free(refcount);
+		s->obj_free(*obj);
+		*obj = cur->key;
+		refcount = cur->datum;
+		*refcount += 1;
+		return 0;
+	} else {
+		free(refcount);
+		return ret;
+	}
+}
+
+static void sepol_objpool_hashtab_free(hashtab_key_t k, hashtab_datum_t d, void *args)
+{
+	struct sepol_objpool *s = args;
+
+	s->obj_free(k);
+	free(d);
+}
+
+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;
+	}
+	return 0;
+}
diff -r 7e5ed01cd8ad libsepol/tests/libsepol-tests.c
--- a/libsepol/tests/libsepol-tests.c	Wed Apr 18 21:32:38 2007 -0400
+++ b/libsepol/tests/libsepol-tests.c	Wed Apr 18 21:32:41 2007 -0400
@@ -24,6 +24,7 @@
 #include "test-deps.h"
 #include "test-list.h"
 #include "test-hashtab.h"
+#include "test-objpool.h"
 
 #include <CUnit/Basic.h>
 #include <CUnit/Console.h>
@@ -65,6 +66,7 @@ static int do_tests(int interactive, int
 	DECLARE_SUITE(deps);
 	DECLARE_SUITE(list);
 	DECLARE_SUITE(hashtab);
+	DECLARE_SUITE(objpool);
 
 	if (verbose)
 		CU_basic_set_mode(CU_BRM_VERBOSE);
diff -r 7e5ed01cd8ad libsepol/tests/test-objpool.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/libsepol/tests/test-objpool.c	Wed Apr 18 21:49:51 2007 -0400
@@ -0,0 +1,117 @@
+/*
+ * Author : Karl MacMillan <kmacmillan@mentalrootkit.com>
+ *
+ * Copyright (C) 2007 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include "test-objpool.h"
+
+#include <sepol/objpool.h>
+#include <sepol/policydb/symtab.h>
+#include <sepol/iter.h>
+#include <sepol/errcodes.h>
+
+#include <stdlib.h>
+#include <stdio.h>
+
+int objpool_test_init(void)
+{
+	return 0;
+}
+
+int objpool_test_cleanup(void)
+{
+	return 0;
+}
+
+static void test_objpool(void)
+{
+	int ret;
+	struct sepol_objpool *objpool;
+	struct sepol_handle *h;
+	char *strs[3];
+	char *str;
+
+	h = sepol_handle_create();
+	CU_ASSERT(h != NULL);
+
+	strs[0] = strdup("foo");
+	CU_ASSERT(strs[0] != NULL);
+	strs[1] = strdup("foo");
+	CU_ASSERT(strs[1] != NULL);
+	strs[2] = strdup("bar");
+	CU_ASSERT(strs[2] != NULL);
+
+	ret = sepol_objpool_create(h, &objpool, (sepol_objpool_hash_t)sepol_symhash,
+				   (sepol_objpool_cmp_t)sepol_symcmp,
+				   free, 64);
+	CU_ASSERT(ret == SEPOL_OK);
+
+	str = strs[0];
+	ret = sepol_objpool_add(h, objpool, (void**)&str);
+	CU_ASSERT(ret == SEPOL_OK);
+	CU_ASSERT(str == strs[0]);
+
+	str = strs[1];
+	ret = sepol_objpool_add(h, objpool,(void**) &str);
+	CU_ASSERT(ret == SEPOL_OK);
+	CU_ASSERT(str == strs[0]);
+
+	str = strs[2];
+	ret = sepol_objpool_add(h, objpool, (void**)&str);
+	CU_ASSERT(ret == SEPOL_OK);
+	CU_ASSERT(str == strs[2]);
+
+	ret = sepol_objpool_del(h, objpool, "foo");
+	CU_ASSERT(ret == SEPOL_OK);
+
+
+	ret = sepol_objpool_del(h, objpool, "foo");
+	CU_ASSERT(ret == SEPOL_OK);
+
+	ret = sepol_objpool_del(h,objpool, "foo");
+	CU_ASSERT(ret == SEPOL_ENOENT);
+
+
+	ret = sepol_objpool_del(h, objpool, "bar");
+	CU_ASSERT(ret == SEPOL_OK);
+
+	ret = sepol_objpool_del(h, objpool, "bar");
+	CU_ASSERT(ret == SEPOL_ENOENT);
+
+	strs[2] = strdup("bar");
+	CU_ASSERT(strs[2] != NULL);
+	str = strs[2];
+	ret = sepol_objpool_add(h, objpool, (void**)&str);
+	CU_ASSERT(ret == SEPOL_OK);
+	CU_ASSERT(str == strs[2]);
+
+	ret = sepol_objpool_free(h, objpool);
+	CU_ASSERT(ret == SEPOL_OK);
+
+	sepol_handle_destroy(h);
+}
+
+int objpool_add_tests(CU_pSuite suite)
+{
+	if (NULL == CU_add_test(suite, "test_objpool",
+				test_objpool)) {
+		return -1;
+	}
+
+	return 0;
+}
diff -r 7e5ed01cd8ad libsepol/tests/test-objpool.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/libsepol/tests/test-objpool.h	Wed Apr 18 21:32:41 2007 -0400
@@ -0,0 +1,12 @@
+/* Author : Karl MacMillan <kmacmillan@mentalrootkit.com> */
+
+#ifndef __test_objpool_h__
+#define __test_objpool_h__
+
+#include <CUnit/Basic.h>
+
+int objpool_test_init(void);
+int objpool_test_cleanup(void);
+int objpool_add_tests(CU_pSuite suite);
+
+#endif



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH - policyrep] add objpool to libsepol

[James Antill]

[-- 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 --]

Re: [PATCH - policyrep] add objpool to libsepol

[Karl MacMillan]

On Thu, 2007-04-19 at 00:47 -0400, James Antill wrote:
> 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...
> 

To be fair - this is more general than strings. Of course, if C string
handling were reasonable then I might not have done this.

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

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

I know - that is irritating me. I can look at some internal iterators
headers for stack allocation, but I don't think it will be pretty. I can
also add error codes that indicate retry. Any other suggestions?

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

Well that sucks. I'll change the function prototype. I'll need to do the
same thing for the iterators.

> ...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().
> 

Yep.

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

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

>  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 ;).
> 

Sure. The hashtab API needs some cleanup and encapsulation, but I
decided to leave it for another day.

I'll submit a new patch with the needed changes.

Karl


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH - policyrep] add objpool to libsepol

[James Antill]

[-- 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 --]

Re: [PATCH - policyrep] add objpool to libsepol

[Karl MacMillan]

On Thu, 2007-04-19 at 11:57 -0400, James Antill wrote:
> 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?

It is - as you make changes to the policy tree strings can be removed as
objects are. Actually, as you delete the policy tree the object pool
should empty. It is probably an error if there is anything left in the
pool when the root node in the tree is destroyed. Hmm - maybe I should
change this to add / remove and let the caller allocated and deallocate
all of the objects (see below for why that is inconvenient though).

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

No - that is not quite the intended use.

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

Ahh - but the objects can be arbitrary, so they might be structs that
need something more than 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 :).
> 

Well, the model I was thinking was that the caller was giving ownership
of the object to the pool. The advantage is that the caller simply calls
add and doesn't have to check return codes to see if their copy of the
object should be freed. Otherwise the caller would conditionally free
the object depending on whether another copy already existed. Object
deletion would have the same problem.

> > > > +	} 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)

It shouldn't.

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

If it did return an error you don't know whether you are leaking or not
- likely things are just screwed, which is why I punt and return the
error for the caller to figure out how to handle :) Eventually the
hashtab API can be fixed so that this is not an issue.

Karl


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH - policyrep] add objpool to libsepol

[James Antill]

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

On Thu, 2007-04-19 at 12:11 -0400, Karl MacMillan wrote:
> On Thu, 2007-04-19 at 11:57 -0400, James Antill wrote:

> >  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().
> 
> Ahh - but the objects can be arbitrary, so they might be structs that
> need something more than 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 :).
> 
> Well, the model I was thinking was that the caller was giving ownership
> of the object to the pool. The advantage is that the caller simply calls
> add and doesn't have to check return codes to see if their copy of the
> object should be freed.

 Well it needs to check for errors, and do something different then.

>  Otherwise the caller would conditionally free
> the object depending on whether another copy already existed. Object
> deletion would have the same problem.

 Yeh, I understand ... it feels kind of wrong to be mixing layers. Ie.
conceptually you seem to have:

struct higher_layer {
  void *(*make)(void); /* allocate */
  struct lower_layer {
    void (*free)(void *); /* deallocate */
  } lower;
};

...which I don't see often, and so assumed was wrong ... but it works,
you obviously meant to do it, and I can't see a quick way to split the
layers without downsides.

-- 
James Antill <jantill@redhat.com>

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