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

[Wyllys Ingersoll]

Jan Pechanec wrote:
> 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.,
>   

I disagree with a lot of our cstyle rules.  I don't understand why it 
should be a problem to use a
negation operator with strlen, the result is obvious.  The semantics of 
strlen (or strcmp either)
are never going to change.  The internet would probably collapse in a 
smoldering heap if it
did :)


>>> 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.
>
>   
Oh, I see what you are saying now.  Yes, I will change it to be freed in 
pk_gencert instead.

-Wyllys