[kmf-discuss] kmf code review (part I.)

[Jan Pechanec]

On Tue, 12 Feb 2008, Wyllys Ingersoll wrote:

>> 1133         if (ekuliststr == NULL || !strlen(ekuliststr))
>>
>> 	- I think we should not use strlen() output as boolean value. It's not
>> so we should check for != 0
>
> I don't see why that is dangerous or confusing, its just more of a personal
> preference, plus it
> helps avoid cstyle errors by running over the 80 column limit in some cases.

	yeah, unfortunately cstyle script doesn't catch everything, see page 
23. It can't recognize if int return value is treated as a boolen or not:

http://www.opensolaris.org/os/community/documentation/getting_started_docs/cstyle.ms.pdf

	Never use the boolean negation operator (!) with non-boolean
	expressions. In particular, never use it to test for a NULL pointer or
	to test for success of the strcmp function, e.g.,
	...


>> 1117                         kmf_free_data(&ekus->ekulist[i]);
>>  	- hmm, I think this is wrong. You are trying to free a piece of memory
>> by pieces. On line 1104 you assign the value so I think that line 1117 should
>> not be there at all since the following line takes care of everything:
>>
>> 1119                 free(ekus->ekulist);
>>  
> No, that takes care of freeing the list of OIDs, but not the actual data within
> each OID.

	right, I see it now.

>> 232         free_eku_list(ekulist);
>>
>> 	- I don't like too much that we put a dynamically allocated memory to a
>> function and expect it to free it then. It's just, hmm, not used very often.
>> Well, it's internal function but it's the only parameter that is alloated
>> outside and freed inside, other are not - subname, for example.
>>  
>
> I agree, but the problem with an unbounded list is that you never know how much
> to
> allocate.  If I pre-allocate a list, it will undoubtedly get broken some day by
> a user who puts 1 too many on the list. 

	hmm, I don't understand. Do we talk about the same thing? What I 
mean is that I suggest that "ekulist" is freed in pk_gencert() because 
that's where it is allocated, not inside of those other functions that 
consume the ekulist.

>> 172-180
>> 399-407
>> 599-606
>>
>> 	- could we have a helper function for that?
>>  
> I don't think its necessary in that case, its is just a simple loop around a
> single macro.  If there

	well, OK, my point was just that it would be less code in total.

	J.

-- 
Jan Pechanec