[SSSD] [PATCH] Remove useless assignment to function parameter

Stephen Gallagher sgallagh at redhat.com
Mon Mar 2 14:19:52 UTC 2015


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150302/13589282/attachment.sig>


More information about the sssd-devel mailing list