Re: [PATCH] HttpRequest::helperNotes to NotePairs

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 07 2013 - 13:54:50 MDT