Re: [POLICYREP PATCH] Add objset to libsepol

[James Antill <jantill@redhat.com>]

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

On Wed, 2007-04-25 at 17:58 -0400, Karl MacMillan wrote:
> Add the objset data structure to libsepol. Object sets behave similarly to
> Python sets.

 Umm, From: http://docs.python.org/lib/types-set.html

        A set object is an unordered collection of immutable values.

...AIUI this will be an ordered collection of mutable values, though
documented to maybe be unordered at some point?

> Signed-off-by: Karl MacMillan <kmacmillan@mentalrootkit.com>
> ---
> 
>  libsepol/include/sepol/objset.h |  169 +++++++++++++++++++++++++++++++++++++++
>  libsepol/src/objset.c           |  154 ++++++++++++++++++++++++++++++++++++

 One thing I thought when reviewing Josh's mamoth patch is that it might
be nice for review to post the unit tests seperately and mark them
clearly. Just a thought.

> diff --git a/libsepol/src/objset.c b/libsepol/src/objset.c
> new file mode 100644
> index 0000000..7b86e6b
> --- /dev/null
> +++ b/libsepol/src/objset.c
> @@ -0,0 +1,154 @@
> +struct sepol_objset
> +{
> +	struct sepol_list *objs;
> +	sepol_objset_cmp_t cmp;
> +	unsigned int len;
> +	char compliment;
> +};
> +
> +int sepol_objset_create(struct sepol_handle *h, struct sepol_objset **set,
> +			sepol_objset_cmp_t cmp)
> +{
> +	int ret;
> +
> +	*set = calloc(1, sizeof(struct sepol_objset));
> +	if (*set == NULL)
> +		return SEPOL_ENOMEM;
> +
> +	(*set)->cmp = cmp;
> +
> +	ret = sepol_list_create(h, &(*set)->objs);
> +	if (ret < 0) {
> +		free(set);

 This should be free(*set).

> +		return ret;
> +	}
> +	
> +	return SEPOL_OK;
> +}
> +
> +int sepol_objset_add(struct sepol_handle *h, struct sepol_objset *s, void *obj)
> +{
> +	int ret, ret2, cmp;
> +	struct sepol_iter *iter;
> +	void *cur;
> +
> +	ret = sepol_iter_create(h, &iter);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sepol_list_begin(h, s->objs, iter);
> +	sepol_foreach(h, ret, cur, iter) {
> +		cmp = s->cmp(s, cur, obj);
> +		if (cmp == 0) {
> +			ret = SEPOL_EEXIST;
> +			goto out;
> +		} else if (cmp > 0) {
> +			ret = sepol_list_insert(h, s->objs, iter, obj);
> +			goto out;
> +		}

 This implies an ordering, so you can't for Eg. add some elements and
then change the cmp function.

> +	}
> +	if (ret != SEPOL_ITERSTOP) {
> +		ERR(h, "iteration ended abnormally");
> +		goto out;
> +	}
> +	ret = sepol_list_append(h, s->objs, obj);
> +out:
> +	ret2 = sepol_iter_free(h, iter);
> +	if (ret2 != SEPOL_OK) {
> +		ERR(h, "error freeing iterator");
> +		return ret2;
> +	}

 I think it's much more likely that you want to know ret and not ret2
here, if ret==SEPOL_EEXIST etc.

> +	return ret;
> +}
> +
> +int sepol_objset_del(struct sepol_handle *h, struct sepol_objset *s, void *obj)
> +{
> +	int ret, ret2, cmp;
> +	struct sepol_iter *iter;
> +	void *cur;
> +
> +	ret = sepol_iter_create(h, &iter);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sepol_list_begin(h, s->objs, iter);
> +	sepol_foreach(h, ret, cur, iter) {
> +		cmp = s->cmp(s, cur, obj);
> +		if (cmp == 0) {
> +			ret = sepol_list_del(h, s->objs, iter);
> +			goto out;
> +		}
> +	}
> +	if (ret == SEPOL_ITERSTOP)
> +		ret = SEPOL_ENOENT;

 You aren't using the ordering here, why is that?

> +		
> +out:
> +	ret2 = sepol_iter_free(h, iter);
> +	if (ret2 != SEPOL_OK)
> +		return ret2;

 Dito. Also no ERR() call?

> +	return ret;	
> +}
> +void sepol_objset_set_compliment(struct sepol_objset *s, char compliment)
> +{
> +	s->compliment = compliment;
> +}
> +
> +char sepol_objset_get_compliement(struct sepol_objset *s)
> +{
> +	return s->compliment;
> +}

 Spillink not soo god?;)

 Also if you are going to add these, I think you want at least
sepol_objset_contains_obj() which does the right thing.

-- 
James Antill <jantill@redhat.com>

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