Re: svn commit: samba r14257 - in trunk/source/passdb: .
On Sun, Mar 12, 2006 at 11:09:32PM +, [EMAIL PROTECTED] wrote: Author: idra Date: 2006-03-12 23:09:31 + (Sun, 12 Mar 2006) New Revision: 14257 WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=revroot=sambarev=14257 Log: commit some fixes to the previous patch as Volker pointed out some flaws. Still has problems. *Never* use talloc_free, always use TALLOC_FREE. If you're using talloc_free you need to be re-examining your patch. Jeremy.
Re: svn commit: samba r14257 - in trunk/source/passdb: .
On Sun, 2006-03-12 at 15:48 -0800, Jeremy Allison wrote: On Sun, Mar 12, 2006 at 11:09:32PM +, [EMAIL PROTECTED] wrote: Author: idra Date: 2006-03-12 23:09:31 + (Sun, 12 Mar 2006) New Revision: 14257 WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=revroot=sambarev=14257 Log: commit some fixes to the previous patch as Volker pointed out some flaws. Still has problems. *Never* use talloc_free, always use TALLOC_FREE. If you're using talloc_free you need to be re-examining your patch. no, the use of talloc_free() is ok because we are always sure the context passed is not null and valid. Only function misused talloc_free() and turned out that function should not call any free at all as it does not own the memory context. (thanks, your comment made me notice that while checking my statements are indeed correct :) Simo. -- Simo Sorce Samba Team GPL Compliance Officer email: [EMAIL PROTECTED] http://samba.org
Re: svn commit: samba r14257 - in trunk/source/passdb: .
On Sun, Mar 12, 2006 at 07:12:03PM -0500, simo wrote: On Sun, 2006-03-12 at 15:48 -0800, Jeremy Allison wrote: On Sun, Mar 12, 2006 at 11:09:32PM +, [EMAIL PROTECTED] wrote: Author: idra Date: 2006-03-12 23:09:31 + (Sun, 12 Mar 2006) New Revision: 14257 WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=revroot=sambarev=14257 Log: commit some fixes to the previous patch as Volker pointed out some flaws. Still has problems. *Never* use talloc_free, always use TALLOC_FREE. If you're using talloc_free you need to be re-examining your patch. no, the use of talloc_free() is ok because we are always sure the context passed is not null and valid. It's a habit thing. If you *always* use TALLOC_FREE you'll have less bugs than if you think you can get away with talloc_free as 'it doesn't matter this time'. I just fixed an unbelievably subtle bug where someone used a _free call instead of a _FREE style call. No one visually inspecting the code would have (or did) see it. Coverity found it. I know it doesn't matter in this case, but using TALLOC_FREE instead here doesn't hurt either, and so it's safe to just always use TALLOC_FREE. If I do a grep on the source and see talloc_free, instead of TALLOC_FREE, I want it to be an exceptional case *only* with a comment explaining why that was used. Jeremy.
Re: svn commit: samba r14257 - in trunk/source/passdb: .
On Sun, 2006-03-12 at 16:18 -0800, Jeremy Allison wrote: On Sun, Mar 12, 2006 at 07:12:03PM -0500, simo wrote: On Sun, 2006-03-12 at 15:48 -0800, Jeremy Allison wrote: On Sun, Mar 12, 2006 at 11:09:32PM +, [EMAIL PROTECTED] wrote: Author: idra Date: 2006-03-12 23:09:31 + (Sun, 12 Mar 2006) New Revision: 14257 WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=revroot=sambarev=14257 Log: commit some fixes to the previous patch as Volker pointed out some flaws. Still has problems. *Never* use talloc_free, always use TALLOC_FREE. If you're using talloc_free you need to be re-examining your patch. no, the use of talloc_free() is ok because we are always sure the context passed is not null and valid. It's a habit thing. If you *always* use TALLOC_FREE you'll have less bugs than if you think you can get away with talloc_free as 'it doesn't matter this time'. I just fixed an unbelievably subtle bug where someone used a _free call instead of a _FREE style call. No one visually inspecting the code would have (or did) see it. Coverity found it. Yes, I'm following the streams of patches. Do you know what I thought when I saw it? That would not have happened if we had a talloc hierarchy. I know it doesn't matter in this case, but using TALLOC_FREE instead here doesn't hurt either, and so it's safe to just always use TALLOC_FREE. See my argumentation on another mail/ If I do a grep on the source and see talloc_free, instead of TALLOC_FREE, I want it to be an exceptional case *only* with a comment explaining why that was used. But I see you feel strongly (reading another reply while answering) .. in that case why not just make talloc_free check for context not being null itself instead of adding a really ugly (visually because of all caps) macro ? :-) Simo. -- Simo Sorce Samba Team GPL Compliance Officer email: [EMAIL PROTECTED] http://samba.org
Re: svn commit: samba r14257 - in trunk/source/passdb: .
On Sun, Mar 12, 2006 at 07:22:18PM -0500, simo wrote: Yes, I'm following the streams of patches. Do you know what I thought when I saw it? That would not have happened if we had a talloc hierarchy. A talloc hierarchy hung off random pointers is a poor man's C++ (IMHO). It can be horribly misued. I'm wary of them at the moment. But I see you feel strongly (reading another reply while answering) .. in that case why not just make talloc_free check for context not being null itself instead of adding a really ugly (visually because of all caps) macro ? :-) Consistency with the Samba4 talloc. It's the Samba3 style here, I'd like to keep it consistent. Jeremy.
Re: svn commit: samba r14257 - in trunk/source/passdb: .
On Sun, 2006-03-12 at 16:32 -0800, Jeremy Allison wrote: On Sun, Mar 12, 2006 at 07:22:18PM -0500, simo wrote: Yes, I'm following the streams of patches. Do you know what I thought when I saw it? That would not have happened if we had a talloc hierarchy. A talloc hierarchy hung off random pointers is a poor man's C++ (IMHO). It can be horribly misued. I'm wary of them at the moment. But I see you feel strongly (reading another reply while answering) .. in that case why not just make talloc_free check for context not being null itself instead of adding a really ugly (visually because of all caps) macro ? :-) Consistency with the Samba4 talloc. It's the Samba3 style here, I'd like to keep it consistent. No TALLOC_FREE() actually adds something good, it makes the pointer NULL after it is freed. This is very important in a code base like that of samba3 which have not grown up with talloc as a programming paradigm, I have not realized that before reading the last mails. So for samba3 I think you are right, and TALLOC_FREE() does really make sense to be used. I'll commit a fix for that in the next hours, gotta go now. Simo. -- Simo Sorce Samba Team GPL Compliance Officer email: [EMAIL PROTECTED] http://samba.org
