Re: [PATCH] Logging of honored certificate validation error names

 On Mon, 07 Nov 2011 13:48:46 -0700, Alex Rousskov wrote:
> On 11/05/2011 04:15 PM, Amos Jeffries wrote:
>> On 2/11/2011 11:13 p.m., Tsantilas Christos wrote:
>>> Currently the %err_detail access_log formating code does not
>>> display
>>> something useful for the system admin in the case of the
>>> certificate
>>> validation errors.
>>>
>>> This patch in the case of an ERR_SECURE_CONNECT_FAIL error displays
>>> the certificate validation error name.
>>>
>>
>> +1. Looks okay.
>>
>> I'm a little dubious about passing request->detailError() the SSL
>> error
>> code instead of the errno. But have no strong objections.
>
> Error detailing code was specifically designed to record
> context-specific details beyond errno (which was already available in
> most cases) and the request->detailError() method itself is usually
> used
> to store non-errno details:
>
>> ./src/Server.cc: request->detailError(ERR_ICAP_FAILURE,
>> ERR_DETAIL_RESPMOD_BLOCK_LATE);
>> ./src/client_side_request.cc:
>> request->detailError(ERR_ACCESS_DENIED, ERR_DETAIL_REQMOD_BLOCK);
>> ./src/client_side_request.cc:
>> request->detailError(ERR_ICAP_FAILURE,
>> ERR_DETAIL_CLT_REQMOD_RESP_BODY);
>> ./src/client_side_request.cc:
>> request->detailError(ERR_ICAP_FAILURE, errDetail);
>
>
> We even document HttpRequest::errDetail to be errType-specific:
>
>> err_type errType;
>> int errDetail; ///< errType-specific detail about the
>> transaction error
>
>
> Why are you dubious about passing request->detailError() the SSL
> error code?
>

 That in this case we seem to have both an errno and an extended error
 with values. Its not clear to me whether this change is keeping the
 system errno around for the report tokens which display pure errno (or
 '-') rather than our extended err_type.

 Not a major issue since I think our err_type is a clearer message
 anyway.

 Amos
Received on Mon Nov 07 2011 - 22:58:53 MST