Re: [PATCH] log format tokens upgrade

On 11/05/2011 05:13 AM, Amos Jeffries wrote:
> This updates the format parser and storage objects in the Format::
> namespace and separates some into separate files for SourceLayout
> project dependency reductions.
>
> Functionally it adds a registration API so components can register
> themselves an array of tokens in a namespace. Registering the arbitrary
> namespace "example" with some tokens ("a","b") will cause the parser to
> accept those tokens in a logging format like so: "%example::a %example::b".
>
> Due to time limitations resolving the library-library symbol
> perculiarities in GCC I have had to cut short of moving the "adapt::"
> and "icap::" tokens into their libraries. But have gone so far as to
> make them use the registration process in this patch ready for that step.
>
> Future work:
> - convert the error pages to use format for the page body macros
> - convert the %ssl_* tokens in src/ssl/* to use format and the
> namespace "ssl::"
> - convert external_acl_type to use formats for its helper input string.
>
> Amos

> + explicit TokenNamespace(const String &nsName, TokenTableEntry const *tSet) : prefix(nsName), tokenSet(tSet) {};

No "explicit" for constructors with more than one required parameter
because they cannot be activated implicitly.

> + ~TokenNamespace() {};

Extra semicolon.
Not needed as the default destructor is the same.

> + /// array of tokens inside this namespace
> + TokenTableEntry const *tokenSet;

AFAICT, 3rd-party code modifies this set. Should we prohibit copying and
assigning of TokenNamespace objects then?

> +class FmtConfig
> +{
> +public:

The new class needs a description, especially if we declare a global of
its type later:

> +extern FmtConfig TheConfig;

> +#define free_format(X) do{ delete (*X).formats; (*X).formats=NULL; }while(false)

Technically, you need parens around X above.

> - {"<st", LFT_ICAP_BYTES_READ},
> + {">st", LFT_ICAP_BYTES_SENT},
> + {"<st", LFT_ICAP_BYTES_READ},
> {"<bs", LFT_ICAP_BODY_BYTES_READ},
>
> {">h", LFT_ICAP_REQ_HEADER},
> {"<h", LFT_ICAP_REP_HEADER},
>
> {"tr", LFT_ICAP_TR_RESPONSE_TIME},
> - {"tio", LFT_ICAP_IO_TIME},
> + {"tio", LFT_ICAP_IO_TIME},
> {"to", LFT_ICAP_OUTCOME},
> {"Hs", LFT_ICAP_STATUS_CODE},
>
> - {NULL, LFT_NONE} /* this must be last */
> + {NULL, LFT_NONE} /* this must be last */

Whitespace non-changes?

> +class TokenTableEntry {

The new class needs a description. There is a broader description for
"namespace Format" above it, but we should define what this specific
class is for.

> + const char *config;
> + ByteCode_t token_type;
> + int options;

Please document these. They are probably obvious to you, but some of us
need a hand to know what yet another "config" really is in this context.

> === modified file 'src/main.cc'
> --- src/main.cc 2011-11-03 10:02:02 +0000
> +++ src/main.cc 2011-11-05 09:41:52 +0000
> @@ -63,6 +63,7 @@
> #include "event.h"
> #include "EventLoop.h"
> #include "ExternalACL.h"
> +#include "format/Token.h"
> #include "fs/Module.h"
> #include "PeerSelectState.h"
> #include "Store.h"
> @@ -1105,6 +1106,8 @@
> // TODO: pconn is a good candidate for new-style registration
> // PconnModule::GetInstance()->registerWithCacheManager();
> // moved to PconnModule::PconnModule()
> +
> + Format::Token::Init();
> }
>
> if (IamPrimaryProcess()) {

Consider removing this dependency or even Format::Token::Init() as such
by using RunnersRegistry API. I understand you may not have time for
that, but perhaps you can add a corresponding TODO to discourage more
code being added to Format::Token::Init()?

> === renamed file 'src/format/Tokens.h' => 'src/format/Token.h'
> --- src/format/Tokens.h 2011-10-13 17:05:25 +0000
> +++ src/format/Token.h 2011-11-05 08:38:12 +0000
> @@ -1,5 +1,7 @@
> -#ifndef _SQUID_FMT_TOKENS_H
> -#define _SQUID_FMT_TOKENS_H
> +#ifndef _SQUID_FORMAT_TOKEN_H
> +#define _SQUID_FORMAT_TOKEN_H
> +
> +#include "format/TokenTableEntry.h"

Can you avoid this dependency by pre-declaring class TokenTableEntry? It
looks like we are only using a pointer to TokenTableEntry in
src/format/Token.h, no?

The above are all minor polishing touches. I did not see any bugs, and I
cannot say I understand this code well enough to find them.

Thank you,

Alex.
Received on Mon Nov 07 2011 - 20:38:04 MST