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

[Amos Jeffries]

On 11/08/2015 2:20 a.m., Amos Jeffries wrote:
> Here is mk2 of the Formatter class for doing display things to CacheMgr
> report payloads.
> 
> I have removed the ostream parts of Alex proposal. Since these formatter
> could create StoreEntryStream at need if they really had to and the
> formatting API does not require that functionality.
> 
> 
> I've labelled it PATCH, since this is operational code and I would like
> to commit if it passes an audit.
> 
> Leaving more of the Formatter API and report conversions to followup
> patches. This one is scoped at getting the base class existing with
> exemplar use-cases.
> 
> Amos


[

On 11/08/2015 2:29 a.m., Alex Rousskov wrote:
> On 08/09/2015 09:39 PM, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>>
>>> I hope we can come up with a minimal API (only usable for the very basic
>>> output such as one-line notices and such). We will need to discuss and
>>> agree on what that minimum is exactly.
>>>
>>> If YAML support is one of the ultimate goals, then I would propose
>>> something like this:
>>>
>>> // Assembles information for a single Cache Manager "page".
>>> // Supplied information goes into a configured stream.
>>> // Kids implement various information formatting methods.
>>> class Page: public std::ostream {
>>> public:
>>>     Page(std::ostream &os);
>
>> Does this mean you want to go head with PackableStream for the libmem
>> problem ?
>
>
> I have nothing against PackableStream* classes as such. If they are
> useful for something, they should be polished and committed, of course.
>
> I am against making half-way changes to the cache manager output.
> Library dependencies can be changed without changing output.
> Alternatively, we can solve linking problems while switching to the new
> and _final_ output format. If you are doing the legwork, it is your call
> whether to combine the two activities or separate them.
>
>
>> If we plan to get rid of PackableStream / StoreEntryStream in the end
>> product that ctor could take a Packable&.
>>
>>
>> This assumes that the Page object does the actual append() to the
>> Packable it references.
>
> The Page essentially takes and uses PackableStream[Buffer]. There is no
> good reason to keep using C-style printf()s if we have to change the
> output anyway.
>

Ah. Okay. My take-2 accepts Packable& and later developments can
construct a PackableStream member for internal use from that if it needs
one.


>>>     virtual ~Page() {}
>>>
>>>     // a comment
>>>     virtual Page &comment(const SBuf &text) = 0;
>>>
>>
>> I was suggesting just the above bit in the first patch.
>
> Fine with me, but your thoughts below seem to indicate that it is not
> actually what you want to do (or that it is impossible to do using a
> comment()).
>
>
>> Although the immediate uses seem more like notice box / announcements
>> than regular text comment. The display end would highlight them in bold
>> colors or something to attract attention.
>
> Then comment() is not appropriate. It would be appropriate for something
> like "no data collected for this page" or "internal error while
> rendering statistics for this page". That is, for something that scripts
> do not need to interpret (beyond the fact that there no actual data in
> the output).
>
> You may add a Page::announce() method of some sort, of course, but it
> has to be _implemented_ using basic YAML constructs like lists, hashes,
> key:value pairs, comments, etc. Thus, you may need some of the other
> methods from the sketch. This is important: To avoid changing same
> output more than once, we have to use the right constructs in any
> changed code.

YAML block-literal suits notices fine. Whereas comment would be YAML
comment since its only for human use.

Though for mk2 I have listed the existing report syntax in PayloadOldCgi
formatter. AFAICS, the reports that do not already conform to that are
malformed and show up broken in cachemgr.cgi display.

Amos