[Pki-devel] Review Request

[Christina Fu]

Jack, you might have missed this, but could you please give description 
to each parameter to
safe_injection_strcat() in the comment at top?
For comment style, there are some examples in the same file, such as 
get_post_field().

conditional ACK.

thanks!
Christina

On 10/23/2012 01:47 PM, John Magne wrote:
> Portion of patch, to address cfu's issues:
>
> cfu, please review.
>
> https://bugzilla.redhat.com/attachment.cgi?id=632378&action=edit
>
> ----- Original Message -----
> From: "Christina Fu"<cfu at redhat.com>
> To: pki-devel at redhat.com
> Sent: Tuesday, October 23, 2012 9:53:33 AM
> Subject: Re: [Pki-devel] Review Request
>
> The following are my review comments:
>
> - I see you replaced the PL_strcat() calls in mod_tokendb.cpp.  How
> about the other PL_strcat() calls in other files? Do they not present
> any issue?
>
> - The following RA::Debug() message and comment seems a bit confusing
> for me as it seems to declare that it's about to truncate and yet it's
> actually trying to resize:
>
> A::Debug( "safe_injection_strcat, about to truncate, resize injection
> buffer:  ", "current len: %d expected_len %d data_len: %d
> cur_injection_size %d",current_len, expected_len, cat_data_len,
> *injection_size );
> /* We are going to get truncated!
>
> How about say something like "...resizing to avoid truncation..."
>
> - safe_injection_strcat()
> not sure of the role of the variable "result" as it only gets set once
> at the end, but returned many times in the middle. Unclear if 1 is
> success or 0 is. There is no comment (yeah, could you please provide
> comment for parameters and return value?). It is especially confusing
> when check_injection_size() comment says "returns 0 on success, 1 on
> failure" and yet safe_injection_strcat() has "if (check_res == 1),
> return result", where result was init'd to be 0,  So does that mean it
> is the opposite of the check_injection_size()?
> Another thing is the callers ignore the return values from
> safe_injection_strcat() anyway, so why return anything at all?
>
> - Why only call check_injection_size 2nd time if still not enough?
>
> Christina
>
> On 10/22/2012 02:54 PM, John Magne wrote:
>> Request review for issue:
>>
>>
>> Bug 864607 - Empty certificate search in TPS results in httpd.worker segmentation fault then server error.
>>
>> Bugzilla:
>> https://bugzilla.redhat.com/show_bug.cgi?id=864607
>>
>> Attachment:
>>
>> https://bugzilla.redhat.com/attachment.cgi?id=630239&action=diff
>>
>> _______________________________________________
>> Pki-devel mailing list
>> Pki-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel