ehlo,
I found this patch in my old branches. It still applies to sssd and remove dead code.
LS
On 02/27/2015 05:27 PM, Lukas Slebodnik wrote:
ehlo,
I found this patch in my old branches. It still applies to sssd and remove dead code.
LS
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello Lukas,
what I don't like about your patch is the fact that you are changing the intentions of the author of the original code.
Still there's no way how to fix function to do what was intended because we can't change the type of the parameter. At least not for sssd_krb5_locator_close() and free_exp_data().
I'm not sure whether we can modify definition of hbac_free_info(). Do you know?
If we are not able to change the functions to do what was intended (which we definitely can't for 2 out of 3) then I agree with you patch.
ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html
Thanks!
On (28/02/15 22:24), Pavel Reichl wrote:
On 02/27/2015 05:27 PM, Lukas Slebodnik wrote:
ehlo,
I found this patch in my old branches. It still applies to sssd and remove dead code.
LS
Hello Lukas,
what I don't like about your patch is the fact that you are changing the intentions of the author of the original code.
I did not change anything. It is dead code and it's very likely compiler optimize it out anyway.
There is a BIG difference between intention of author and what code does :-)
Still there's no way how to fix function to do what was intended because we can't change the type of the parameter. At least not for sssd_krb5_locator_close() and free_exp_data().
I'm not sure whether we can modify definition of hbac_free_info(). Do you know?
We cannot change API of hbac_free_info either. It is part of public API $ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free 0000000000001030 T hbac_free_info
If we are not able to change the functions to do what was intended (which we definitely can't for 2 out of 3) then I agree with you patch.
ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html
LS
On 03/02/2015 10:02 AM, Lukas Slebodnik wrote:
On (28/02/15 22:24), Pavel Reichl wrote:
On 02/27/2015 05:27 PM, Lukas Slebodnik wrote:
ehlo,
I found this patch in my old branches. It still applies to sssd and remove dead code.
LS
Hello Lukas,
what I don't like about your patch is the fact that you are changing the intentions of the author of the original code.
I did not change anything. It is dead code and it's very likely compiler optimize it out anyway.
There is a BIG difference between intention of author and what code does :-)
Yes, I'm well aware of that.
Still there's no way how to fix function to do what was intended because we can't change the type of the parameter. At least not for sssd_krb5_locator_close() and free_exp_data().
I'm not sure whether we can modify definition of hbac_free_info(). Do you know?
We cannot change API of hbac_free_info either. It is part of public API $ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free 0000000000001030 T hbac_free_info
OK, thanks.
ACK (ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html)
If we are not able to change the functions to do what was intended (which we definitely can't for 2 out of 3) then I agree with you patch.
ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:02 AM, Lukas Slebodnik wrote:
On (28/02/15 22:24), Pavel Reichl wrote:
On 02/27/2015 05:27 PM, Lukas Slebodnik wrote:
ehlo,
I found this patch in my old branches. It still applies to sssd and remove dead code.
LS
Hello Lukas,
what I don't like about your patch is the fact that you are changing the intentions of the author of the original code.
I did not change anything. It is dead code and it's very likely compiler optimize it out anyway.
There is a BIG difference between intention of author and what code does :-)
Yes, I'm well aware of that.
Still there's no way how to fix function to do what was intended because we can't change the type of the parameter. At least not for sssd_krb5_locator_close() and free_exp_data().
I'm not sure whether we can modify definition of hbac_free_info(). Do you know?
We cannot change API of hbac_free_info either. It is part of public API $ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free 0000000000001030 T hbac_free_info
OK, thanks.
ACK (ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html)
Did either of you reach out to the original authors to see if they're OK with the removal? If not, can you?
On 03/02/2015 10:38 AM, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:02 AM, Lukas Slebodnik wrote:
On (28/02/15 22:24), Pavel Reichl wrote:
On 02/27/2015 05:27 PM, Lukas Slebodnik wrote:
ehlo,
I found this patch in my old branches. It still applies to sssd and remove dead code.
LS
Hello Lukas,
what I don't like about your patch is the fact that you are changing the intentions of the author of the original code.
I did not change anything. It is dead code and it's very likely compiler optimize it out anyway.
There is a BIG difference between intention of author and what code does :-)
Yes, I'm well aware of that.
Still there's no way how to fix function to do what was intended because we can't change the type of the parameter. At least not for sssd_krb5_locator_close() and free_exp_data().
I'm not sure whether we can modify definition of hbac_free_info(). Do you know?
We cannot change API of hbac_free_info either. It is part of public API $ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free 0000000000001030 T hbac_free_info
OK, thanks.
ACK (ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html)
Did either of you reach out to the original authors to see if they're OK with the removal? If not, can you?
OK, I'll do it, but I'm not sure if one of the authors is Yassir.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Mar 02, 2015 at 10:41:22AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:38 AM, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:02 AM, Lukas Slebodnik wrote:
On (28/02/15 22:24), Pavel Reichl wrote:
On 02/27/2015 05:27 PM, Lukas Slebodnik wrote:
ehlo,
I found this patch in my old branches. It still applies to sssd and remove dead code.
LS
Hello Lukas,
what I don't like about your patch is the fact that you are changing the intentions of the author of the original code.
I did not change anything. It is dead code and it's very likely compiler optimize it out anyway.
There is a BIG difference between intention of author and what code does :-)
Yes, I'm well aware of that.
Still there's no way how to fix function to do what was intended because we can't change the type of the parameter. At least not for sssd_krb5_locator_close() and free_exp_data().
I'm not sure whether we can modify definition of hbac_free_info(). Do you know?
We cannot change API of hbac_free_info either. It is part of public API $ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free 0000000000001030 T hbac_free_info
OK, thanks.
ACK (ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html)
Did either of you reach out to the original authors to see if they're OK with the removal? If not, can you?
OK, I'll do it, but I'm not sure if one of the authors is Yassir.
Stephen wrote the HBAC evaluator, Sumit created both the krb5 plugin and pam_sss.
On Mon, Mar 02, 2015 at 10:43:36AM +0100, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:41:22AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:38 AM, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:02 AM, Lukas Slebodnik wrote:
On (28/02/15 22:24), Pavel Reichl wrote:
On 02/27/2015 05:27 PM, Lukas Slebodnik wrote: >ehlo, > >I found this patch in my old branches. It still applies to sssd and remove dead >code. > >LS > > Hello Lukas,
what I don't like about your patch is the fact that you are changing the intentions of the author of the original code.
I did not change anything. It is dead code and it's very likely compiler optimize it out anyway.
There is a BIG difference between intention of author and what code does :-)
Yes, I'm well aware of that.
Still there's no way how to fix function to do what was intended because we can't change the type of the parameter. At least not for sssd_krb5_locator_close() and free_exp_data().
I'm not sure whether we can modify definition of hbac_free_info(). Do you know?
We cannot change API of hbac_free_info either. It is part of public API $ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free 0000000000001030 T hbac_free_info
OK, thanks.
ACK (ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html)
Did either of you reach out to the original authors to see if they're OK with the removal? If not, can you?
OK, I'll do it, but I'm not sure if one of the authors is Yassir.
Stephen wrote the HBAC evaluator, Sumit created both the krb5 plugin and pam_sss.
Hi,
since both the krb5 locator plugin and pam_sss are loaded into other programs I wanted to make sure to clean up as much as possible to avoid issues in the caller even if the caller misbehaves.
Although I see the point that this might hide issues in the caller I think it would makes sense to stay defensive here.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Mar 02, 2015 at 11:27:09AM +0100, Sumit Bose wrote:
On Mon, Mar 02, 2015 at 10:43:36AM +0100, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:41:22AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:38 AM, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:02 AM, Lukas Slebodnik wrote:
On (28/02/15 22:24), Pavel Reichl wrote: >On 02/27/2015 05:27 PM, Lukas Slebodnik wrote: >>ehlo, >> >>I found this patch in my old branches. It still applies to sssd and remove dead >>code. >> >>LS >> >> >Hello Lukas, > >what I don't like about your patch is the fact that you are changing the >intentions of the author of the original code. > I did not change anything. It is dead code and it's very likely compiler optimize it out anyway.
There is a BIG difference between intention of author and what code does :-)
Yes, I'm well aware of that.
>Still there's no way how to fix function to do what was intended because we >can't change the type of the parameter. At least not for >sssd_krb5_locator_close() and free_exp_data(). > >I'm not sure whether we can modify definition of hbac_free_info(). Do you >know? > We cannot change API of hbac_free_info either. It is part of public API $ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free 0000000000001030 T hbac_free_info
OK, thanks.
ACK (ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html)
Did either of you reach out to the original authors to see if they're OK with the removal? If not, can you?
OK, I'll do it, but I'm not sure if one of the authors is Yassir.
Stephen wrote the HBAC evaluator, Sumit created both the krb5 plugin and pam_sss.
Hi,
since both the krb5 locator plugin and pam_sss are loaded into other programs I wanted to make sure to clean up as much as possible to avoid issues in the caller even if the caller misbehaves.
Although I see the point that this might hide issues in the caller I think it would makes sense to stay defensive here.
I'm sorry, Pavel pointed out to me that we only clear the local copy of the pointer and not the origin (we only get * not **). In this case the assignment is useless.
bye, Sumit
bye, Sumit
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
On (02/03/15 14:39), Sumit Bose wrote:
On Mon, Mar 02, 2015 at 11:27:09AM +0100, Sumit Bose wrote:
On Mon, Mar 02, 2015 at 10:43:36AM +0100, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:41:22AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:38 AM, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:02 AM, Lukas Slebodnik wrote: >On (28/02/15 22:24), Pavel Reichl wrote: >>On 02/27/2015 05:27 PM, Lukas Slebodnik wrote: >>>ehlo, >>> >>>I found this patch in my old branches. It still applies to sssd and remove dead >>>code. >>> >>>LS >>> >>> >>Hello Lukas, >> >>what I don't like about your patch is the fact that you are changing the >>intentions of the author of the original code. >> >I did not change anything. It is dead code and it's very likely compiler >optimize it out anyway. > >There is a BIG difference between intention of author and what code does :-) Yes, I'm well aware of that. >>Still there's no way how to fix function to do what was intended because we >>can't change the type of the parameter. At least not for >>sssd_krb5_locator_close() and free_exp_data(). >> >>I'm not sure whether we can modify definition of hbac_free_info(). Do you >>know? >> >We cannot change API of hbac_free_info either. It is part of public API >$ nm --defined-only --dynamic /usr/lib64/libipa_hbac.so | grep free >0000000000001030 T hbac_free_info OK, thanks.
ACK (ci passed:http://sssd-ci.duckdns.org/logs/job/8/43/summary.html)
Did either of you reach out to the original authors to see if they're OK with the removal? If not, can you?
OK, I'll do it, but I'm not sure if one of the authors is Yassir.
Stephen wrote the HBAC evaluator, Sumit created both the krb5 plugin and pam_sss.
Hi,
since both the krb5 locator plugin and pam_sss are loaded into other programs I wanted to make sure to clean up as much as possible to avoid issues in the caller even if the caller misbehaves.
Although I see the point that this might hide issues in the caller I think it would makes sense to stay defensive here.
I'm sorry, Pavel pointed out to me that we only clear the local copy of the pointer and not the origin (we only get * not **). In this case the assignment is useless.
That's exactly reason why cppcheck reported it as dead assignment.
LS
On Mon, 2015-03-02 at 14:43 +0100, Lukas Slebodnik wrote:
On (02/03/15 14:39), Sumit Bose wrote:
On Mon, Mar 02, 2015 at 11:27:09AM +0100, Sumit Bose wrote:
On Mon, Mar 02, 2015 at 10:43:36AM +0100, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:41:22AM +0100, Pavel Reichl wrote:
On 03/02/2015 10:38 AM, Jakub Hrozek wrote:
On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl wrote: > On 03/02/2015 10:02 AM, Lukas Slebodnik wrote: > > On (28/02/15 22:24), Pavel Reichl wrote: > > > On 02/27/2015 05:27 PM, Lukas Slebodnik wrote: > > > > ehlo, > > > > > > > > I found this patch in my old branches. It still > > > > applies to sssd and remove dead > > > > code. > > > > > > > > LS > > > > > > > > > > > Hello Lukas, > > > > > > what I don't like about your patch is the fact that > > > you are changing the intentions of the author of the > > > original code. > > > > > I did not change anything. It is dead code and it's > > very likely compiler optimize it out anyway. > > > > There is a BIG difference between intention of author > > and what code does :-) > Yes, I'm well aware of that. > > > Still there's no way how to fix function to do what > > > was intended because we > > > can't change the type of the parameter. At least not > > > for sssd_krb5_locator_close() and free_exp_data(). > > > > > > I'm not sure whether we can modify definition of > > > hbac_free_info(). Do you > > > know? > > > > > We cannot change API of hbac_free_info either. It is > > part of public API $ nm --defined-only --dynamic > > /usr/lib64/libipa_hbac.so | grep free 0000000000001030 > > T hbac_free_info > OK, thanks. > > ACK (ci passed:http://sssd- > ci.duckdns.org/logs/job/8/43/summary.html) Did either of you reach out to the original authors to see if they're OK with the removal? If not, can you?
OK, I'll do it, but I'm not sure if one of the authors is Yassir.
Stephen wrote the HBAC evaluator, Sumit created both the krb5 plugin and pam_sss.
Hi,
since both the krb5 locator plugin and pam_sss are loaded into other programs I wanted to make sure to clean up as much as possible to avoid issues in the caller even if the caller misbehaves.
Although I see the point that this might hide issues in the caller I think it would makes sense to stay defensive here.
I'm sorry, Pavel pointed out to me that we only clear the local copy of the pointer and not the origin (we only get * not **). In this case the assignment is useless.
That's exactly reason why cppcheck reported it as dead assignment.
Yeah, this looks like it was excessive defense, not useful functionality. Go ahead and kill it.
sssd-devel@lists.fedorahosted.org