On 12/14/2012 02:49 PM, Tomas Babej wrote: > On 12/14/2012 01:59 PM, Alexander Bokovoy wrote: >> 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: >>>> > I see I forgot to react to this one. This construct is not introduced by this > patch, > and anyway, I personally like it, because it is an easy way of expressing > that the > condition is satisfied if and only if ipabaserid and ipasecondarybaserid are > equivalent. > >>>> 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). >> > Ok, I replaced rid range for RID range on all occasions. > > Updated patch attached :) > > Tomas
Ok, this looks good. I just removed extraneous parens in "if (...):" as we agreed to. ACK. Pushed to master, ipa-3-1. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel