[kmf-discuss] KMF code review (part II.)

[Wyllys Ingersoll]

Huie-Ying Lee wrote:
> Wyllys Ingersoll wrote:
>
>> Thanks for the comments so far.   The webrev will be updated shortly.
>>
>> -Wyllys
>>
>
> Here is my code review comments for the updated webrev.
>
> signcsr.c: yn_to_int() (line 50-64)
>
>   Do we really need the yn_to_int() function ?
>   I think we can reuse the yesno() function in common.c for the same 
> purpose.

Agree, I moved it to common.c and changed yesno() to also use it.
>
> signcsr.c: write_cert_file()
>
>   This function is not needed, because the caller can use the existing 
> API
>   kmf_create_cert_file(const KMF_DATA *, KMF_ENCODE_FORMAT, char *);

OK

>
> kmfapi.h: line 327
>
>   Why change kmf_select_token() to be a public API?  
> kmfapiP.h: line 354

I thought Jan needed it in his code, if he does not, then I will keep it 
private.
>
>   Nit; Consider rename eku_present() to is_eku_present(), because I think
>   is_eku_present() sounds better. 8-)
OK
>
> kmftypes.h: line 331
>
>   Not needed.  This line is really a duplicate of line 329.

OK, not sure why I added that one. Strange.

>
> csrcrlop.c, kmf_decode_csr()
>
>   line 521, does the csrdata has enough space ?  Should it be checked ?

csrdata and cdata are both structures of type KMF_CSR_DATA, so they must be
the same size.

>
> certop.c:
>
>   The kmf_sign_data() API needs to be updated to check the key usage 
> policy
>   as well.

It does - see line 583
> pem_encode.c: get_line(), line 474, 479 and 482
>
>   Nit; it might be better to get rid of the unnecessary variable - "len".
>   This function can just return "i" in the end.

OK

>
> pkcs11_spi.c: line 2737 and 2738
>
>   Nit; It might be better if combining these 2 lines into 1 line, such 
> as        if (cred != NULL && cred->credlen > 0)

OK

Thanks for the comments, the webrev has been updated:

http://cr.opensolaris.org/~wyllys/kmf

-Wyllys