On Fri, 2011-04-08 at 10:29 -0400, Rob Crittenden wrote: > Martin Kosek wrote: > > On Fri, 2011-04-01 at 13:51 -0400, Rob Crittenden wrote: > >> Martin Kosek wrote: > >>> Target branches: master, ipa-2-0 > >>> --- > >>> > >>> Most of the pwpolicy_* commands do include cospriority in the result > >>> and potentially in the attribute rights (--all --rights). Especially > >>> when --raw output is requested. This patch fixes it for all > >>> pwpolicy commands. > >>> > >>> https://fedorahosted.org/freeipa/ticket/1103 > >>> > >> > >> nack. I see a couple of problems. > >> > >> You should test for rights before doing the cosentry_show(). If rights > >> is False then we won't add the data whatever it is so it is more > >> efficient to exit earlier. > > > > We have to call cosentry_show every time (except for the case when we > > pull data for the global policy) because we read cospriority attribute. > > But the function was indeed not efficient (it called cosentry_show > > twice), I rewrote it. > > > >> > >> Same with pwpolicy_name == global_policy_name. I think you should drop > >> the try/except and make it: > >> > >> if not rights or pwpolicy_name == global_policy_name: > >> return > >> > >> ... > >> > >> It should never be the case that the cosentry is not found so I'd let it > >> fail if that does occur. > > > > Fixed. > > > >> > >> I think that keys[-1] can be None so be aware. > > > > Fixed. > > > >> > >> You hardcode rights == False in pwpolicy_find(), a good thing. I think > >> you should add make it explict rights=False and add a comment explaining > >> that you can't get accessrights with a find. > > > > Fixed. > > > > Fixed patch attached. > > > > Martin > > Looks great, ack. > > rob
Pushed to master, ipa-2-0. _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
