Vladimir Kotal wrote: > mark powers wrote: >> kcf_callprov.c >> -------------- >> mcp-0 line 119 I don't think the assert is that useful since >> key != NULL is checked before the function is >> called. > > I added it just for the case of someone calling > kcf_check_prov_mech_keylen() in the future without checking the key > pointer first. It is just a safety belt. It's not necessary so I have > removed it. > >> mcp-0 lines 122-129 This code is duplicated three times in this >> function. It could be moved to before the >> actual check on line 180. > > Those instances are not the same. For CRYPTO_KEY_RAW key types it uses > key->ck_length, for CRYPTO_KEY_ATTR_LIST it has to get the attribute > length first. Also, CRYPTO_KEY_RAW key lengths are converted from bits > to bytes using the CRYPTO_BITS2BYTES macro. In fact, they are doing > different conversions - shifting versus multiplication. Lastly, moving > the whole block (assuming generic macro for both types) before the > switch statement would be suboptimal for CRYPTO_KEY_REFERENCE key type > and other types. > > I can make a macro just for the CRYPTO_KEY_ATTR_LIST key type > assignments where there is indeed a duplication - would this be okay ?
I think the macro would be overkill since it would only be used twice. You're right that the code is more complex than I thought. However, I think you could factor out mech_info = &(KCF_TO_PROV_MECHINFO(provider, mech_type)); and move to just before the switch. > >> mcp-3 Related to Krishna's comment (KY-1), wouldn't it be better to >> always return CRYPTO_KEY_SIZE_RANGE instead of >> CRYPTO_MECH_NOT_SUPPORTED? > > This is easy to do for kcf_get_hardware_provider() in case the > provider is not logical provider. As for the other cases, I am not > sure - the key length check is now part of provider selection (it's > one of the criteria for selection) so from this perspective it should > fall under CRYPTO_MECH_NOT_SUPPORTED. Seems to me that more descriptive error codes should trump generic ones, but perhaps that is outside the scope of this bug fix. > > Incremental webrev (on top of already approved changes) is here: > http://cr.opensolaris.org/~vkotal/kcf-keylen_check-6786946.onnv.Mark/ > > > v.
