[Freeipa-devel] [PATCH 0021] Forbid overlapping rid ranges for the same id range

[Martin Kosek]

On 12/13/2012 11:52 AM, Tomas Babej wrote:
> On 12/12/2012 04:32 PM, Martin Kosek wrote:
>> On 10/26/2012 03:43 PM, Tomas Babej wrote:
>>> Hi,
>>>
>>> creating an id range with overlapping primary and secondary
>>> rid range using idrange-add or idrange-mod command now
>>> raises ValidationError. Unit tests have been added to
>>> test_range_plugin.py.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3171
>>>
>>> Tomas
>>>
>> 1) Add command can cause crash:
>>
>> # ipa idrange-add range9 --base-id=1000 --rid-base=1000 --secondary-rid-base=
>> --range-size 1000
>> ipa: ERROR: an internal error has occurred
>>
>> 2) I don't like this construct very much:
>>
>> updated_values = dict(zip(rid_range_attributes,[None]*3))
>>
>> This would look better, IMO:
>> updated_values = dict((attr, None) for attr in rid_range_attributes)
>>
>> Why do you need this dict pre-created anyway? You overwrite all keys here:
>>
>> +            for attr in rid_range_attributes:
>> +                if attr in entry_attrs:
>> +                    updated_values[attr] = entry_attrs[attr]
>> +                else:
>> +                    updated_values[attr] = int(old_attrs[attr][0])
>>
>>
>> 3) [nitpick] We don't end ValidationError with '.':
>>
>> +                   raise errors.ValidationError(name='ID Range setup',
>> +                       error=_("Primary rid range and secondary rid range"\
>> +                               " cannot overlap."))
>>
>> There is also a duplication of the same error message...
>>
>> 4) The -mod operation will also crash:
>>
>> # ipa idrange-add range9 --base-id=1000 --rid-base=1000
>> --secondary-rid-base=2000 --range-size 1000
>> # ipa idrange-mod range9 --secondary-rid-base=
>> ipa: ERROR: an internal error has occurred
>>
>> Martin
> 
> New patch version as well as diff between
> patch versions (for your convenience) attached.
> 
> Tomas

1) You introduced mixed spaces and tabs - Python gods do not like that!

2) This is a nitpick, but there are too many redundant parens and brackets in
this statement:

+	if(any([attr is None for attr in [rid_base,secondary_rid_base, size]])):
+            return False

This would look nicer and would not create unnecessary list:

+	if any(attr is None for attr in (rid_base, secondary_rid_base, size)):
+            return False

3) Another construct I did not like very much:

+	is_set = lambda x : (x in entry_attrs) and not (x is None)

a) "x is not None" reads better than "not (x is None)"
b) I would rather replace all is_set lambdas with use of "if
entry_attrs.get('attribute')" which is also used in other places in ipalib

4) I see a suspicions check

+            if (is_set('ipasecondarybaserid') != is_set('ipabaserid')):

I though that ipasecondarybaserid is optional. With your change it is not:

# ipa idrange-add range9 --base-id=1000 --rid-base=1000 --range-size 1000
ipa: ERROR: invalid 'ID Range setup': Options secondary_rid_base and rid_base
must be used together

It is also quite ugly condition, I would do something like:

if entry_attrs.get('ipasecondarybaserid') and not entry_attrs.get('ipabaserid'):
... raise error

5) I would not create a list when it is not necessary, a tuple is more
efficient I think:

+            rid_range_attributes =
['ipabaserid','ipasecondarybaserid','ipaidrangesize']

6) If we want to check user does not create secondary RID range without a
primary RID range, we should also do it in -mod operation:

# ipa idrange-mod range9 --rid-base=
--------------------------
Modified ID range "range9"
--------------------------
  Range name: range9
  First Posix ID of the range: 1000
  Number of IDs in the range: 1000
  First RID of the secondary RID range: 2000
  Range type: local domain range

7) I am sorry I did not come with this in my previous review, but I have one
more nitpick for the error message:
+                       error=_("Primary rid range and secondary rid range"\
+                               " cannot overlap"))

I would do s/rid/RID/ as we also refer it as RID in our help...

Martin