On Tue, Oct 16, 2012 at 02:26:24PM +0200, Tomas Babej wrote:
> Hi,
> 
> commands ipa idrange-add / idrange-mod no longer allows the user
> to enter primary or secondary rid range such that has non-zero
> intersection with primary or secondary rid range of another
> existing id range, as this could cause collision.
> 
> Unit tests added to test_range_plugin.py
> 
> https://fedorahosted.org/freeipa/ticket/3086
> 
> Tomas

Thank you for the patch, comments are in-line.

bye,
Sumit

> >From a46a8d0aa4e64e105a53a177b6a12cf28e56620e Mon Sep 17 00:00:00 2001
> From: Tomas Babej <tba...@redhat.com>
> Date: Mon, 15 Oct 2012 06:28:16 -0400
> Subject: [PATCH] Forbid overlapping primary and secondary rid ranges
> 
> Commands ipa idrange-add / idrange-mod no longer allows the user
> to enter primary or secondary rid range such that has non-zero
> intersection with primary or secondary rid range of another
> existing id range, as this could cause collision.
> 
> Unit tests added to test_range_plugin.py
> 
> https://fedorahosted.org/freeipa/ticket/3086
> ---
>  .../ipa-range-check/ipa_range_check.c              |  93 +++++++++++++---
>  tests/test_xmlrpc/test_range_plugin.py             | 120 
> +++++++++++++++++++--
>  2 files changed, 191 insertions(+), 22 deletions(-)
> 
> diff --git a/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c 
> b/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
> index 
> 499e54a9c4a4c9134a231c0cd09e700390565a14..4f9f7437d11d2bc33238b14f5099a42b4c5463d2
>  100644
> --- a/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
> +++ b/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
> @@ -132,24 +132,67 @@ done:
>      return ret;
>  }
>  
> -#define IN_RANGE(x,base,size) ( (x) >= (base) && ((x) - (base)) < (size) )
> -static bool ranges_overlap(struct range_info *r1, struct range_info *r2)
> +#define IN_RANGE(x,base,size) ( (x) >= (base) && (x) < (size+base) )

Would you mind to use the original definition of IN_RANGE? x-base looks
a bit odd, but I made it on purpose to avoid overruns. Since we already
know that x>=base we can be sure that there will be no underrun.

> +static bool intervals_overlap(uint32_t x, uint32_t base, uint32_t x_size, 
> uint32_t base_size)
>  {
> -    if (r1->name != NULL && r2->name != NULL &&
> -        strcasecmp(r1->name, r2->name) == 0) {
> -        return false;
> -    }
> -
> -    if (IN_RANGE(r1->base_id, r2->base_id, r2->id_range_size) ||
> -        IN_RANGE((r1->base_id + r1->id_range_size - 1), r2->base_id, 
> r2->id_range_size) ||
> -        IN_RANGE(r2->base_id, r1->base_id, r1->id_range_size) ||
> -        IN_RANGE((r2->base_id + r2->id_range_size - 1), r1->base_id, 
> r1->id_range_size)) {
> +    if (IN_RANGE(x, base, base_size) ||
> +        IN_RANGE((x + x_size - 1), base, base_size) ||
> +        IN_RANGE(base, x, x_size) ||
> +        IN_RANGE((base + base_size - 1), x, x_size)) {
>          return true;
>      }
>  
>      return false;
>  }
>  
> +//returns 0 if there is no overlap
> +//connected ranges must not overlap:
> +//  existing range:  base  rid  sec_rid
> +//                    |     |  \  / |
> +//                    |     |   \/  |
> +//                    |     |   /\  |
> +//                    |     |  /  \ |
> +//  new range:       base  rid  sec_rid

I think we currently do not use C++ style comments in freeipa C code.
Can you switch to /* */ comments?

> +static int ranges_overlap(struct range_info *r1, struct range_info *r2)
> +{
> +    if (r1->name != NULL && r2->name != NULL &&
> +        strcasecmp(r1->name, r2->name) == 0) {
> +        return 0;
> +    }
> +
> +    //check if base range overlaps with existing base range
> +    if (intervals_overlap(r1->base_id, r2->base_id,
> +        r1->id_range_size, r2->id_range_size)){
> +        return 1;
> +    }
> +
> +    //if both base_rid and secondary_base_rid are 0, the rid range is not set
> +    //in that case we skip the primary/secondary rid range overlap test
> +    if((r1->base_rid!=0 || r1->secondary_base_rid!=0) &&
> +       (r2->base_rid!=0 || r2->secondary_base_rid!=0)){

can you add spaces around '!=' ?

> +
> +        //check if rid range overlaps with existing rid range
> +        if (intervals_overlap(r1->base_rid, r2->base_rid,
> +            r1->id_range_size, r2->id_range_size))
> +            return 2;
> +
> +        //check if secondary rid range overlaps with existing secondary rid 
> range
> +        if (intervals_overlap(r1->secondary_base_rid, r2->secondary_base_rid,
> +            r1->id_range_size, r2->id_range_size))
> +            return 3;
> +
> +        //check if rid range overlaps with existing secondary rid range
> +        if (intervals_overlap(r1->base_rid, r2->secondary_base_rid,
> +            r1->id_range_size, r2->id_range_size))
> +            return 4;
> +
> +        //check if secondary rid range overlaps with existing rid range
> +        if (intervals_overlap(r1->secondary_base_rid, r2->base_rid,
> +            r1->id_range_size, r2->id_range_size))
> +            return 5;
> +    }

Return code is missing if one of the ranges does not have the rid ranges
set. Can you add a test case for this condition as well?

> +}
> +
>  static int ipa_range_check_start(Slapi_PBlock *pb)
>  {
>      return 0;
> @@ -176,7 +219,7 @@ static int ipa_range_check_pre_op(Slapi_PBlock *pb, int 
> modtype)
>      int search_result;
>      Slapi_Entry **search_entries = NULL;
>      size_t c;
> -    bool overlap = true;
> +    int no_overlap = 0;
>      const char *check_attr;
>      char *errmsg = NULL;
>  
> @@ -315,13 +358,31 @@ static int ipa_range_check_pre_op(Slapi_PBlock *pb, int 
> modtype)
>              goto done;
>          }
>  
> -        overlap = ranges_overlap(old_range, new_range);
> +        no_overlap = ranges_overlap(new_range, old_range);
>          free(old_range);
>          old_range = NULL;
> -        if (overlap) {
> -            LOG_FATAL("New range overlaps with existing one.\n");
> +        if (no_overlap != 0) {
>              ret = LDAP_CONSTRAINT_VIOLATION;
> -            errmsg = "New range overlaps with existing one.";
> +            if (no_overlap==1) {
> +                errmsg = "New base range overlaps with existing base range.";
> +                LOG_FATAL("New base range overlaps with existing base 
> range.\n");
> +            }
> +            if (no_overlap==2) {
> +                errmsg = "New primary rid range overlaps with existing 
> primary rid range.";
> +                LOG_FATAL("New primary rid range overlaps with existing 
> primary rid range.\n");
> +            }
> +            if (no_overlap==3) {
> +                errmsg = "New secondary rid range overlaps with existing 
> secondary rid range.";
> +                LOG_FATAL("New secondary rid range overlaps with existing 
> secondary rid range.\n");
> +            }
> +            if (no_overlap==4) {
> +                errmsg = "New primary rid range overlaps with existing 
> secondary rid range.";
> +                LOG_FATAL("New primary rid range overlaps with existing 
> secondary rid range.\n");
> +            }
> +            if (no_overlap==5) {
> +                errmsg = "New secondary rid range overlaps with existing 
> primary rid range.";
> +                LOG_FATAL("New secondary rid range overlaps with existing 
> primary rid range.\n");
> +            }

Can you change this to a switch block? Please also add a default
section. This was new return codes (for new checks) can be added and we
still get a message even if it does not have a matching message here.

LOG_FATAL can handle arguments, so I would suggest to add a

LOG_FATAL("%s\n", errmsg);

after the switch block.

>              goto done;
>          }
>      }

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to