On Thu, Sep 05, 2013 at 05:17:24PM +0200, Pavel Březina wrote: > On 09/05/2013 03:09 PM, Simo Sorce wrote: > >On Thu, 2013-09-05 at 10:49 +0200, Pavel Březina wrote: > >>On 09/04/2013 09:12 PM, Simo Sorce wrote: > >>>On Wed, 2013-09-04 at 17:06 +0200, Sumit Bose wrote: > >>>>On Tue, Sep 03, 2013 at 02:07:29PM -0400, Simo Sorce wrote: > >>>>>On Tue, 2013-09-03 at 15:25 +0200, Pavel Březina wrote: > >>>>>>On 09/03/2013 01:27 PM, Sumit Bose wrote: > >>>>>>>Hi, > >>>>>>> > >>>>>>>while looking at expand_ccname_template() becasue of shadowing rewind() > >>>>>>>I realized that there a some issues with some of the new krb5.conf > >>>>>>>templates. This patch fixes them and adds some tests to avoid similar > >>>>>>>issues in the future. > >>>>>>> > >>>>>>>There is one change in behaviour. If the name in the %{} braces does > >>>>>>>not > >>>>>>>match any of the known krb5.conf templates for UNIX the new code > >>>>>>>returns > >>>>>>>an error while the old just returned something, which in most case will > >>>>>>>not be the original input. Please tell me if you prefer the original > >>>>>>>input in this case so that I can change the patch accordingly. > >>>>>> > >>>>>>I think we should expand the parameters we can, i.e. uid and euid and > >>>>>>leave everything else intact (i.e. don't even check ${LIBDIR} etc.). Ad > >>>>>>absurdum: for what we know, users can ran custom build that supports > >>>>>>other variables... > >>>>> > >>>>>True, I was looking at this code yesterday again as I am hadling 2071 > >>>>>and I was thinking the same, we should just ignore and leave unchanged > >>>>>%{xyz} patterns we do not understand, so that if libkrb5 adds something > >>>>>the admin can use we do not have to explicitly support it. > >>>>> > >>>>>>But if you don't agree, I'll ack this patch. > >>>>> > >>>>>No, let's nack, and if you all do not mind I can come up with an > >>>>>alternative patch that does what I describe above. > >>>> > >>>>Whatever you prefer, but I can do the needed changes as well. If you do > >>>>them, please do not forget to add tests for the cases which are > >>>>currently not covered. > >>> > >>>I opened bug #2076 to track this issue and attached find a different > >>>approach with testes against current master. > >>> > >>>Simo. > >> > >>Nack. > >> > >>krb5_utils-tests.c: In function ‘test_krb5_style_expansion’: > >>krb5_utils-tests.c:679:9: error: unused variable ‘ret’ > >> > >>It looks good otherwise. > >>Can we also include Sumit's tests? > > > >Sumit's tests consider any 'unknown' expansion variable as invalid, so > >they would fail. > > > >Attached new version w/o ret and rebased on current master. > > > >Simo. > > OK then. Ack.
Pushed to master and sssd-1-10 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel