229289 - Unused variable 'i' in pk11_DestroySessionObjectData(), lib/softoken/pkcs11u.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

In lib/softoken/pkcs11u.c, rev. 1.53, function
pk11_DestroySessionObjectData(), the local variable 'i'
is unused if neither PKCS11_STATIC_ATTRIBUTES nor
PKCS11_REF_COUNT_ATTRIBUTES is defined.

Comment 1

[Wan-Teh Chang]

Attachment: (Av1) warning fix [Checked in: Comment 7]

I hope there is a more elegant way to fix this compiler
warning.

Comment 2

[Serge Gautherie (:sgautherie)]

The point is whether the two defines can be true at the same time or not ?


If they can, as the current code implies,
I see 3 possibilities:

Y1) 2 inside blocks like "{ int i; for( i=...) {...} }"
which you did not accept in bug 90906.

Y2) double #ifdef
which you propose, and seems satisfactory/best.

Y3) 2 inside blocks like "{ for( int i=...) {...} }"
which is a simple "enhancement" of my Y1 proposal :->

In Y1 and Y3, the outer block delimiters are needed in case the compiler deals
with the |for| in the "old-style".


If they can't, the best way becomes

N1) use #elif, combined with Y3 without the outer block delimiters.

Comment 3

[Robert Relyea]

I don't know where the code implies that they can both be set? That fact is they
can't. There are 3 (not 4) allocation strategies. The following comment is from
pkcs11i.h where these values are defined.

-----------------------------------------

/*
 *  Attribute Allocation strategy:
 *
 * 1) static allocation (PKCS11_STATIC_ATTRIBUTES set
 *                       PKCS11_REF_COUNT_ATTRIBUTES not set)
 *   Attributes are pre-allocated as part of the session object and used from
 *   the object array.
 *
 * 2) heap allocation with ref counting (PKCS11_STATIC_ATTRIBUTES not set
 *                                      PKCS11_REF_COUNT_ATTRIBUTES set)
 *   Attributes are allocated from the heap when needed and freed when their
 *   reference count goes to zero.
 *
 * 3) arena allocation (PKCS11_STATIC_ATTRIBUTES  not set
 *                       PKCS11_REF_COUNT_ATTRIBUTE not set)
 *   Attributes are allocated from the arena when needed and freed only when
 *   the object goes away.
 */
#define PKCS11_STATIC_ATTRIBUTES
/*#define   PKCS11_REF_COUNT_ATTRIBUTES          */

----------------------------------------------------------

moot arguments:

If they both could have been defined, I like Wan-Teh's solution the best of the
options given. Y1 is counter to NSS style (NSS prefers not to create lots of
nexted blocks used for nothing more than variable declarations), and Y3 isn't
valid C code (It's C++ and wouldn't necessarily compile in .c files).

I'm not sure what you mean by 'using elif' (again, using 'Y3' doesn't work on
'C' code). You could just simply pull in the declarations to the top of the
#if's, but I don't like this solution because it could break if there is some
future additional code added at the 'top' of this function (remember this is 'C'
code, and all variables must be declared at the beginning of the function). So
if we must silence the warnings, then I suggest taking Wan-Teh's patch.

BTW on the PKCS11_STATIC_ATTRIBUTES define gets regular testing. 

bob

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) trivial code cleanup

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 138452 [details] [diff] [review]
(Bv1) trivial code cleanup


I have no compiler: Could you compile/test/review it ? Thanks.

Comment 6

[Serge Gautherie (:sgautherie)]

Re comment 3:

Thanks for your explanation(s);
I stand corrected on C/C++ syntax; (then, do go for (Y2) Av1 patch !)
What I meant by implies/#elif is "solved" by Bv1 patch...

Comment 7

[Wan-Teh Chang]

Comment on attachment 137890 [details] [diff] [review]
(Av1) warning fix
[Checked in: Comment 7]

I checked in this patch on the NSS trunk.

Comment 8

[Wan-Teh Chang]

Comment on attachment 138452 [details] [diff] [review]
(Bv1) trivial code cleanup

This patch seems to consist of adding or deleting
blank lines and changing #else and #ifdef to
#elif defined.	All of these changes are a matter
of personal taste.  So I recommend not taking this
patch.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 138452 [details] [diff] [review]
(Bv1) trivial code cleanup


*blank lines:
(I could prepare another patch without them)

*#elseif:
In my point of view, it simplifies the code;
and make it more explicit: having separate #if (can)"implies" that the two
blocks could be true at the same time :-(
(Let's wait for 'r?'.)

Comment 10

[Wan-Teh Chang]

The fix for the compiler warning was also checked into
the NSS_3_9_BRANCH, so I'm marking this bug fixed with
target milestone NSS 3.9.1.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> (From update of attachment 138452 [details] [diff] [review])
> 
> (Let's wait for 'r?'.)

What about this proposed patch then ?

Comment 12

[Wan-Teh Chang]

Serge, the purpose of this bug is to fix the
compiler warning.  It has been fixed.

The #elif changes in your patch might be worth
taking.  I'll let Bob decide.  You should attach
a new patch without the white space changes.  In
any case, it is just code cleanup and is unrelated
to the compiler warning I originally reported.

Comment 13

[Robert Relyea]

Comment on attachment 138452 [details] [diff] [review]
(Bv1) trivial code cleanup

OK, I took too long getting to this... the r- is for one trivial matters, which
if corrected the code can be checked in.

The patch tries to factor out the ending '}' around lines
2232/2228 and 2248/2241. If you change just this, the patch is approved without
further review (including white space changes).

1) The code is easier to read and understand if each if the ending brace is not
factored out, and

2) factoring the brace out will confuse vi's brace matching algorithms.