On Fri, 14 Dec 2012, Tomas Babej wrote:
On 12/13/2012 02:48 PM, Martin Kosek wrote:
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!
Oops, I'd rather send a fixed patch sooner than they bring down their wrath on me. :)
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
Brackets reduced.
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
I don't think this approach would be beneficial. If any of rid_base,
secondary_rid_base, base_id, e.g. would be set to 0, the expression like

/entry_attrs.get('base_id')

/would be evaluated as False both in case that there is no key 'base_id'
in the dictionary and in the case that the value associated with the key
is 0. To avoid these problems, we would have to complicate conditions
used in if-s, and therefore make the readability worse.
/
/
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
It's not a bug. It's a feature :)

Secondary base RID indeed is mandatory when primary RID base has been defined. Its purpose is to prevent collisions when user and group share the same POSIX ID number.

From the documentation of idrange.py:

/To create an ID range for the local domain it is not necessary to specify a// //domain SID. But since it is possible that a user and a group can have the same//
//value as Posix ID a second RID interval is needed to handle conflicts.

/
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']
Fixed.
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
This is fixed as part of my patch 0024 as it falls under the scope of
http://fedorahosted.org/freeipa/ticket/3170

I will send a rebased version later today.
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
Fixed. However, lower-case rid is used in ipa_range_check.c 389 plugin.
We might want to consider filing a naming convention ticket then.
RID is RID as it is abbreviation of Relative ID.
See http://msdn.microsoft.com/en-us/library/cc246018.aspx for details of
SID (and RID as it is part of SID).

--
/ Alexander Bokovoy

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

Reply via email to