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.
bye, Sumit
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...
But if you don't agree, I'll ack this patch.
bye, Sumit
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.
Simo.
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.
bye, Sumit
Simo.
-- Simo Sorce * Red Hat, Inc * New York
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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.
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?
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.
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.
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@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Simo Sorce -
Sumit Bose