On Thu, Nov 05, 2015 at 10:29:41AM +0100, Lukas Slebodnik wrote: > On (05/11/15 09:50), Pavel Reichl wrote: > > > > > >On 11/05/2015 08:59 AM, Lukas Slebodnik wrote: > >>On (04/11/15 18:58), Pavel Reichl wrote: > >>> > >>> > >>>On 11/04/2015 06:41 PM, Sumit Bose wrote: > >>>>On Wed, Nov 04, 2015 at 06:00:34PM +0100, Pavel Reichl wrote: > >>>>> > >>>>> > >>>>>On 11/04/2015 03:02 PM, Lukas Slebodnik wrote: > >>>>>>On (04/11/15 14:40), Pavel Reichl wrote: > >>>>> > >>>>>>> > >>>>>>>Also one of few not yet tested corners of pam_reply() - 'Printing > >>>>>>>account > >>>>>>>expiration warning for sshd' requires pam_service to be sshd, but I can > >>>>>>>change when I will write another test (it might happen soon as pcech is > >>>>>>>working on similar change in adjacent code). > >>>>>>> > >>>>>>That's exactly the reason why the default value should not be sshd. > >>>>> > >>>>>I don't understand why this would be the reason can you elaborate? > >>>> > >>>>Because a default value should not lead to a code path which handles a > >>>>special case. The general PAM responder test should not run through the > >>>>'sshd' case in pam_reply() only if the service is set explicitly to > >>>>'sshd' this features should be tests. > >>> > >>>Sure, but just setting sshd would not lead to execution of this branch > >>>AFAIK > >>>at least pam verbosity would be needed to be set. As said before service > >>>value doesn't matter in current test so IMO this patch would only set > >>>service > >>>to more sane default however I agree with you that 'generic service name' > >>>is > >>>not a bad idea at all. > >>> > >>We spent enormous time with such unimportant patch. > >> > >>I pushed patch with Sumit's propsal "pam_test_service" > >>using one-liner rule. > > > >So this rule really allows you to: > > > >1) amend patch yourself, > >2) change authorship of the patch, > Sumit proposed "pam_test_service" so he is author.
Pavel proposed to change the service name first. > > >3) ack the patch yourself, > yes > > >4) push the patch yourself, > yes > > But only for oneliners yes, this rule exists but imo it should be only used to un-break build of to fix other urgent issues [*] and not to cut a discussion short. Especially since Pavel already said that he like the idea of a generic name and to me it sounded like he will include this in the next version of his patch set. bye, Sumit [*] I haven't checked all commits where this rule was used and I'm pretty sure there will be examples where only a typo is fixed. Nevertheless I think the rule is there to allow the swift handling of emergencies and not to make it easy to commit minor change in general. > > LS > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
