[squid-dev] [PATCH] PayloadFormatter (was PackableStream)

[Alex Rousskov]

On 08/12/2015 06:28 AM, Amos Jeffries wrote:
> On 12/08/2015 5:07 a.m., Alex Rousskov wrote:
>> On 08/10/2015 08:20 AM, Amos Jeffries wrote:
>>> Here is mk2 of the Formatter class for doing display things to CacheMgr
>>> report payloads.
>>
>> Please post the patch (your post had no attachments), preferably
>> reflecting the discussion that happened since then.


> Oops. Sorry.


Unfortunately this patch does not seem to reflect recent discussions. I
am not sure how to restart them efficiently, but here are a few specific
comments:


> +/// Base API for organizing the processing of a compiled cache manager command.
> +/// Not a job because all methods are synchronous (but they may start jobs).
> +class PayloadFormatter

Wrong class description (belongs to Action, not this class).

I do not think we should call this class Mgr::PayloadFormatter because
it formats high-level [cache manager] reports and "payload" is commonly
used to refer to very different (and usually low-level) things.
Moreover, formatting may be just one of the things this class does. I
believe something like Mgr::Page, Mgr::Report or, if you really want a
Formatter suffix, Mgr::Page/ReportFormatter would work much better.

The same applies to other changes using "payload" terminology.


> +/**
> + * Produces cache manager report payloads in a human-readable markup syntax
> + * which is parsed by the cachemgr.cgi tool.
...
> +class PayloadOldCgi: public PayloadFormatter

I think it is wrong to strongly associate the old format with
cachemgr.cgi. Lot's of admins use those reports without cachemgr.cgi.
The CGI script is just one way to render them. I also would not use the
"Old" suffix because the current format might remain with us forever and
watch other formats become "old".

Alternatives to consider: Mgr::PlainPage or Mgr::PlainReport.


> +class PayloadFormatter
...
> +    Packable &outBuf;
> +};

Since the majority of PayloadFormatter methods and callers are going to
assemble and format pieces of text and numbers, I think this should
become an ostream. Or, if you have very good reasons for using outBuf
here, there could be a method that returns an ostream backed by this outBuf.

Almost all the code that uses outBuf and PayloadFormatter methods in the
patch already looks labored/awkward (often worse than the trivial code
it replaces!), and would look much better if replaced by the usual
ostream "<<" expressions.

This is important to address now so that we do not have to suffer with
an awkward cache manager page writing interface through all the future
Action::dump() rewrites.


> +    // a comment
> +    virtual void notice(const SBuf &) = 0;

Let's try to define the purpose of this method more precisely so that we
know when it is being used [in]correctly. For example:

  /// a free-form comment or informational text that
  /// is not meant to be further parsed by automation tools
  virtual void notice(const SBuf &) = 0;

Return PayloadFormatter reference so that the calls can be chained
together (see earlier discussion for examples).


> +    void notImplemented(const char *name, size_t len) {
> +        outBuf.append(name, len);
> +        outBuf.append(" is Not Implemented",19);
> +    }

Missing documentation for a non-obvious public method. Most likely, you
do not want the second argument because callers will not find it useful.
The current caller does not AFAICT.

The current implementation will violate formatting rules.
PayloadFormatter methods should not write text directly because they do
not know how it is supposed to be formatted! The implementation should
probably just use notice() instead.

Please add "cache manager action" prefix to explain what is not
implemented. I would also rename notImplemented() to
actionNotImplemented() for clarity sake.

Capitalization in "foo is Not Implemented" looks weird to me. It is
better to use lower case letters throughput IMO.


> + * Syntax ABNF:
> + *  payload     = *( table / kv-list / notice / LF )
> + *  table       = [ label ':' [ table-row ] ] 1*( table-row )
> + *  table-row   = 1*( [ string ] '\t' ) string LF
> + *  kv-list     = label ( '=' / ':' ' ' ) string LF
> + *  label       = 1*( VCHAR / SP ) ; any printable excluding ':' and '='
> + *  notice      = string LF
> + *  string      = 1*OCTET    ; any characters excluding LF (\n) and TAB (\t)

This syntax is very ambiguous AFAICT. If it actually matches reality
(and cachemgr.cgi just guesses what the next construct is most likely to
be), then you may want to add a comment about that fact. Same if this
syntax only approximates what Squid actually dumps.

Also, the proposed notice() method says nothing about excluding LF (\n)
and TAB (\t) from comment values. The method should document what we
want to exclude from all notices (if anything).


> +    virtual void displayWith(Mgr::PayloadFormatter &p) ...

This is not really about "display" as the response may still go through
several post-processing layers before it is seen by anybody. Consider
using writeTo() or dumpTo() instead.


> -    virtual void dump(StoreEntry *entry);
> +    virtual void displayWith(Mgr::PayloadFormatter &) override final;

Why is this method and many others marked as "final"? Is there something
wrong with adding a kid class that would dump a longer menu, for example?

AFAICT, the primary intent of using the "final" specification is to
restrict future overriding, not just to document that "nothing is
overriding this method right now". That is, there is a difference
between "cannot be overridden without breaking something" (the "final"
intended purpose) vs "is not currently overridden" (the "final" abuse).

("final" may also be used as a potential performance optimization, but
we do not care about such minor optimizations in this context.)

This is somewhat similar to "throw()" declarations that C++ folks were
initially excited about until they realized that it is actually a severe
restriction (often with bad consequences) rather than just a convenient
documentation trick.


HTH,

Alex.