Re: [PATCH] HttpRequest::helperNotes to NotePairs

On 04/16/2013 06:42 PM, Tsantilas Christos wrote:
> The 5th version of the patch.
> This patch handles Amos comments.

If there is not any objection I will commit the latest patch to trunk.

Regards,
  Christos

>
> Regards,
> Christos
>
>
> On 04/07/2013 04:54 PM, Tsantilas Christos wrote:
>> On 04/07/2013 07:50 AM, Amos Jeffries wrote:
>>> On 6/04/2013 3:27 a.m., Tsantilas Christos wrote:
>>>> This patch try to fix current current Notes interface and usage.
>>>>
>>>> The changes done having in mind that we need:
>>>> 1) to add multiple notes with the same key
>>>> 2) to support 3 different note types: adaptation meta headers, helper
>>>> notes and custom notes added by the system administrator
>>>> 3) to log notes using the %note formatting code
>>>> 4) to use the %note formating code everywhere the formating API is used.
>>>> For example use the %note with the request_header_add configuration
>>>> parameter.
>>>> 5) to use notes with ACLs.
>>>>
>>>>
>>>> The NotePairs class is implemented from scratch. It stores the notes in
>>>> a simple key:value pairs list, which just allow multiple entries with
>>>> the same key. I have an implementation which uses a
>>>> "key:value1,value2,value3..." form but my sense is that the
>>>> implementation I am posting here is simpler and faster.
>>>>
>>>> - The %{key}notes prints a coma separated list of note values which have
>>>> the key as key. Eg the %{user}note will print:
>>>> "J, Smith",chtsanti
>>>>
>>>> - The %notes prints a "\r\n" separated list of key:value pairs:
>>>> user: "J, Smith"\r\nuser: chtsanti\r\n
>>>>
>>>> - The %notes formating code can be used now with request_header_add and
>>>> other similar configuration parameters.
>>>>
>>>> - The Helper response may needs fixing. For example if the helper return:
>>>> user="J, Smith",chtsanti
>>>> The user:"J, Smith,chtsanti" note will be stored. This is because of
>>>> the strwordtok function used to parse responses.
>>>>
>>>> - We may need to merge the AccessLogEntry::notes and
>>>> AccessLogEntry::helperNotes/HttpRequest::helperNotes to the same object.
>>>> The adaptation meta headers maybe can be merged to the same object to.
>>>>
>>>> Some details also exist in the patch
>>>>
>>>>
>>>> Regards,
>>>> Christos
>>>
>>> Hi Christos, thank you for this.
>>>
>>> in src/AccessLogEntry.h:
>>>
>>> * If AccessLogEntry::notes and AccessLogEntry::helperNotes are really
>>> necessary to be separate, please name the *::notes on configNotes or
>>> something more descriptive of what it holds.
>>> - Also for my knowledge, can you explain why they are now separate and
>>> in need of re-combining? they started out as a combined lis
>>
>> I think there is not a reason to have them separated.
>> The adaptation and helpers notes was separated before this patch, and
>> just merged before logged to log file.
>> Maybe there are reasons for this so I let it for a future decision.
>>
>>
>>>
>>>
>>> in src/ConfigParser.* :
>>>
>>> * ConfigParser::QuoteString const-correctness seems unrelated. Please
>>> feel free to apply this change by itself immediately.
>>
>> OK, I will commit with a separate patch.
>>
>>>
>>>
>>> in src/HelperReply.cc:
>>>
>>> * operator<< line if(!r.notes.empty() > 0) - is very obscure. Please
>>> alter the condition to clarify what exactly is being tested.
>>> + Probably the " > 0" is not meant to be there.
>>
>> Oops!
>> Well it works :-) as is, but of course it is a mistake
>>
>>
>>>
>>> * please avoid adding new empty line at the end of file
>> OK.
>>
>>>
>>>
>>> in src/HttpRequest.cc
>>> * we no longer need to check for helperNotes != NULL before setting it
>>> to NULL. Just set.
>>> - same on the copy()
>>
>> OK.
>>
>>>
>>>
>>> in src/Notes.cc
>>> * Notes::add(const String &noteKey) - should be able to be
>>> const_iterator still.
>>
>> Nope. it will not work....
>>
>>>
>>>
>>> NP: thats all I've had time to check for today. More in a few days.
>>
>> I will wait more comments before post a new patch
>>
>> Regards,
>> Christos
>>
>>>
>>> Amos
>>>
>>
>>
>
Received on Sun Apr 28 2013 - 10:21:22 MDT