Re: [PATCH] HttpRequest::helperNotes to NotePairs

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 list.

in src/ConfigParser.* :

* ConfigParser::QuoteString const-correctness seems unrelated. Please
feel free to apply this change by itself immediately.

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.

* please avoid adding new empty line at the end of file

in src/HttpRequest.cc
* we no longer need to check for helperNotes != NULL before setting it
to NULL. Just set.
  - same on the copy()

in src/Notes.cc
* Notes::add(const String &noteKey) - should be able to be
const_iterator still.

NP: thats all I've had time to check for today. More in a few days.

Amos
Received on Sun Apr 07 2013 - 04:50:45 MDT