Re: [PATCH] Detail all X509_V_ERRs

On 03/24/2011 06:09 PM, Alex Rousskov wrote:
> On 03/24/2011 04:48 AM, Tsantilas Christos wrote:
>
>> This simple patch provide %D details for all SSL errors documented at
>> http://www.openssl.org/docs/apps/verify.html
>>
>> This patch also adds a std::map based structure to optimize the ssl
>> error description retrieval.
>
>> + const char *err = getErrorDescr(error_no);
>> + if (!err)
>> + return "[Not available]";
>> + else
>> + return err;
>> +
>
> Consider a shorter/safer version:
>
> if (const char *err = getErrorDescr(error_no))
> return err;
> return "[Not available]";

fixed

>
>
>> +//Use std::map to optimize search
>> +std::map<Ssl::ssl_error_t, SslErrorDetailEntry *> TheSslDetailMap;
> ...
>> + std::map<Ssl::ssl_error_t, SslErrorDetailEntry *>::iterator it;
>> + for (it=TheSslDetailMap.begin(); it != TheSslDetailMap.end(); ++it) {
>> + if (strcmp(name, it->second->name) == 0)
>> + return it->second->value;
>> }
>
> Consider a more polished and slightly safer version:
>
>> typedef std::map<Ssl::ssl_error_t, SslErrorDetailEntry *> SslErrorDetails;
>> static SslErrorDetails TheSslDetailMap;
>> ...
>> typedef SslErrorDetails::const_iterator SEDCI;
>> for (SEDCI i = TheSslDetailMap.begin(); i != TheSslDetailMap.end(); ++i) {
>> ...

fixed

>
>
> The SslErrorDetails type may be used here as well (also correcting for
> const-ness):
>
>> + const SslErrorDetails::const_iterator it = TheSslDetailMap.find(value);
>> + if (it != TheSslDetailMap.end())
fixed
>
>
> I would also rename TheSslDetailMap to TheSslDetails because the code
> does not really care about the exact index structure. For example, when
> hash_map becomes easily portable, we can start using that instead.

The TheSslDetailMap renamed to the TheSslDetails

Also
  - The loadSslDetailMap() now is static
  - I am using "const SslErrorDetail *" to typedef SslErrorDetails:
     typedef std::map<Ssl::ssl_error_t, const SslErrorDetailEntry *>
SslErrorDetails;

>
>
> Thank you,
>
> Alex.
>