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

[Alex Rousskov]

On 08/15/2015 12:20 AM, Amos Jeffries wrote:
> I dont like payload particularly either in this case. But page is wrong.
> 
> Page is what the remote end display tool will be generating. *IF* a
> "page" exists at all.

There are many kinds of payloads. There are many kinds of pages. There
are many kinds of reports. No name is perfect. I am not going to spend
more time trying to explain why your wrong name is wronger than my wrong
name. If you think your naming scheme is better, who am I to argue?
[rhetorical]


>>> +/**
>>> + * 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.

> That was intentional. This format is nasty even for a free-form format.
> You noticed the syntax is ambiguous and commented on it.
> 
> IMO deprecating this while leaving it available is a good choice. If it
> were not for backward compatibiliy and transitional needs I would
> propose removing it entirely (did in fact).
>  Instead I'm starting with this one, and moving on to YAML etc. to
> provide the non-ambiguous outputs.

Your response does not seem to be related to my objection, but again, I
am not going to spend more time arguing about your naming scheme.


> For now this class is specifically and intentionally dumping out the
> (old) format for cachemgr.cgi. Other third-party tools are considered
> only so far as the cachemgr ambiguous syntax was published in various
> parts in various places. As long as the output still matches that they
> should be fine (or better off).

IMO, made-up after-the-fact syntax rules are irrelevant (because
"nobody" knows about them, and they are imprecise). Cachemgr.cgi itself
is irrelevant (because we control it). Admin scripts parsing old output
are relevant. Thus, we should either keep the old output pretty much "as
is" or replace it with something sufficiently better to justify the
change pains for the admins. Adjusting output (while claiming
compatibility with some syntax rules we made up) is a bad approach
because it makes admins unhappy while not making us happy.


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

> You want to go back to PackableStream now?

If possible, the new page/report/payload building interface should be
ostream-enabled, not PackableStream-enabled. IIRC, that's what I
sketched a few emails back.

We do not have to support Packable to provide ostream interface, but if
Packable backing helps, we should certainly use it.

Said that, ostream is the wrong primary interface for assembling
payload/pages/reports. IMO, you should reintroduce ostream capabilities,
but we should not be [going back to] assembling primary
payload/pages/reports using ostream. More on that below.


> Because StoreEntryStream would place a Store.h dependency almost as
> widely as squid.h dependency.

The _implementation_ of the ostream-based interface may use something
like PackableStream or even StoreEntryStream without introducing any
excessive dependencies AFAICT. In the sketch I posted, only one line of
code new about std::cout backing while the rest was using ostream.



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

> Now you see the benefit of ostream.

My attitude towards ostream has not changed. Forming payloads/pages
/reports using ostream as the top-level interface was wrong and still is.

Making ostream available for opaque and low-level parts of payload is a
good approach. My earlier sketch was probably too small to illustrate
this point, but it does provide high-level report structuring methods
while keeping ostream available for low-level stuff. More needs to be
done to polish that split based on actual typical use cases.


>>> +    // 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


> No.
> 
> notice = "A free-form informational text block that is meant for display
> without further processing."
> 
> comment = "A free-form informational text that is not meant for
> automated tool processing or display. May be discarded by automation."

You have documented notice() as "a comment". Now you give two different
definitions for those two words. Please make up your mind and adjust the
notice() description accordingly. After that, it should be clear whether
we agree what notice() is.

Please also adjust the description to emphasize whether a notice() is an
indication of a possible problem/error or can be safely ignored [by
automation tools] without any harm. The distinction would be important
for automation. If there are two kinds of notices (e.g., Warnings and
FYI messages), then we may need to add a notice(kind) parameter or split
notice() into two methods.

For example, "foo is not yet implemented" is probably something that
tools may safely ignore, but "you do not have permission to access foo"
is an error that a tool should treat as such. Should both use the same
notice() method and, hence, be indistinguishable to the automation tool?
Probably not.


>>   virtual void notice(const SBuf &) = 0;
>>
>> Return PayloadFormatter reference so that the calls can be chained
>> together (see earlier discussion for examples).


> IME chaining usually turns out to be a mistake when it comes to regular
> methods like these and needs undoing later when things move beyond
> simplistic cases. I would rather avoid as long as possible.

If you agree that "simplistic cases" benefit from chaining, then I do
not understand why deny them a useful tool? Many cache manager
payloads/pages/reports are pretty simple... Availability of a chaining
API does not force the caller to use it.

The menu example I posted earlier uses chaining and seems to benefit
from chaining. I suspect some other use cases will benefit as well.

However, this issue is not a deal breaker. If you are certain chaining
is harmful, do not add it.


>> Capitalization in "foo is Not Implemented" looks weird to me. It is
>> better to use lower case letters throughput IMO.
>>
> 
> Sorry, old habbit. 501. Fixed.

Now you may want to fix the method description as well. Naming the
formal parameter and using the name in the fixed description text would
help clarify the description IMO. For example:

    /// a 'something is not implemented' notice()
    void notImplemented(const SBuf &something);



>>> + * 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.


> Indeed. You see one reason why I have been so impatient about it.


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


> Yes and no. This combines both what cachemgr.cgi parses (table, notice),
> and what Squid is elsewhere documented as outputting (kv-list). With
> extra detail gleaned from what Squid actually outputs byte-wise in
> reports that use those table/list constructs (kv-list delimiters, tables
> without header row, and empty-lines).
> 
> There are a few reports using syntax like SP-padded columns with no tabs
> which cachemgr.cgi borks display of badly. I count them as wrong reports
> (using notice lines instead of table or kv-list) to be fixed.
> 
> And I've not managed to add block indentation in there yet in a way that
> leaves the syntax readable. So reality is worse than it looks.

Your description matches my expectations so my suggestion still stands:
Document that the ABNF is ambiguous, only approximates the reality, will
not be adhered to (or even guide) future formats, and is provided as an
illustration only.


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

> I don't want to exclude anything. YAML and other syntaxes wont need to
> in the end result.


Your desires and plans only strengthens my suggestion to disclaim the
actual impact of this formal- and precise-looking ABNF.



>>> -    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?
> 
> Yes. 


For the record, I think your reasons do not justify "final" use. They
are "political" reasons, not "code" reasons. However, I am not going to
insist on a complete "final" removal right now.


> I absolutely do not want anything deriving from OldCgi and keeping
> it around longer than it has to be for transition to the better ones.

If you motivation is to prevent derivatives, then you are misplacing
"final". It should go into the class declaration line instead (as
illustrated below). However, since nobody will know why you are abusing
"final" like this, please also add a comment:


  /// ...
  /// Nothing should derive from this class because we are afraid
  /// that any derivative will keep this class around longer.
  class PayloadOldCgi final: public PayloadFormatter


> As to menu. The length of menu, and any report segments appended is
> Action/ActionChild scope. Whose duty it is to fetch the menu data and
> call formatter once per segment type, or simply provide more table rows
> before ending the menu table.

I do not know why the above is relevant, but please remove "final" from
non-OldCgi methods. Overriding them may not be needed right now, but
there is no reason, even a political one, to prohibit such overriding.


Going forward, please do not use "final" unless really necessary. Treat
it like we treat "throw()" declarations.


In summary, the most important top-level problems [I remember] are:

* unavailability of an ostream interface for low-level formatting
* poorly defined notice() scope


Thank you,

Alex.