Closed Bug 505880 Opened 12 years ago Closed 11 years ago
_ZRealloc could use a local NSSArena *arena variable to make coverity happy
coverity tries to watch out for lock abuse, and it can indeed find cases where locks are abused (even i our codebase). sometimes it's helpful to give coverity a helping hand....
Hoes does this help? What problem does it solve?
sorry, i missed the key bit! the analysis looks at the pointer path to the lock As far as it can tell, the locks i'm quoting here with minuses are distinct - PR_Lock(h->arena->lock); + PR_Lock(arena->lock); - PR_Unlock(new_h->arena->lock); + PR_Unlock(arena->lock); but it should agree that the locks we would be using are the same
Comment on attachment 390123 [details] [diff] [review] could we do this Timeless, thank for explaining the issue. I am agreed, in principle, to allowing a change like this to be made to this function. It may even make this code easier to understand by NSS developers. However, I am going to ask you to submit one more version of this patch which contains a stylistic change. Given a pointer variable declared as: someTime * varPtr; long ago, the NSS team had a member who could not bring himself to ever code if (!varPtr) or if (varPtr) Instead, he insisted on coding if ((someType *)NULL == varPtr) or if ((someType *)NULL != varPtr) The NSS team leaders are agreed that the form with explicit casts and comparisons with NULL is MUCH more difficult to read than the simpler form above, and so we are trying to slowly rid NSS of code in that undesired style. So, we ask that, whenever a line of code bearing that style is changed for ANY reason, that it be changed to the simpler style. I'm NOT asking you to do a wholesale conversion of this file to the new style. Please DON'T do that. I'm merely asking you to take the lines that you are already planning to change, which bear that style and change them by changing (someType *)NULL == to just ! and changing (sometType *)NULL != to (empty) When that is done, I think this patch will be ready for r+. I could do that, but then I'd have to find yet another reviewer. If you code it, then I can review it an commit it. Thanks.
Attachment #390123 - Flags: review?(nelson) → review-
1131 new_h = (struct pointer_header *)p; 1132 new_h->arena = h->arena; 1140 h->arena = (NSSArena *)NULL; 1142 PR_Unlock(new_h->arena->lock); So, you're saying that it cannot tell that the value of new_h->arena->lock at line 1142 is the same as the value of h->arena->lock at line 1131? That seems lame. I can imagine this being an issue in many MANY places in NSS, and really wouldn't want to change them all just to quiet coverity. I want to withhold approval of this checkin until we know just how many such changes to NSS are desired to quiet coverity. This patch seems correct and satisfies my style needs. :) I'd give it r+, but since NSS doesn't have separate review and approval flags I'm going to withhold the review flag until we know how many more changes will be needed to satisfy/quiet coverity. If it's too many, I'll just say no to the whole lot of them.
i think this is the only place where it had issues. at least i'm pretty sure i exhausted the set of LOCK complaints. note that i can't commit to nss, so r+ or no r+ it hardly matters :) and yeah, personally i felt this warning was lame. otoh, doing it would also help the compiler save on indirections, so it should actually be a general win, even if the original complaint was from coverity.
Comment on attachment 390198 [details] [diff] [review] new nss style r=nelson. I should commit this soon. Ping me if I don't.
Attachment #390198 - Flags: review?(nelson) → review+
Checking in nss/lib/base/arena.c new revision: 1.13; previous revision: 1.12
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.7
You need to log in before you can comment on or make changes to this bug.