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

Amos Jeffries squid3 at treenet.co.nz
Sat Aug 15 06:20:52 UTC 2015


New patch attached. But only some of the details are changed.

We have some pretty significant items not yet agreed on.


On 13/08/2015 10:19 a.m., Alex Rousskov wrote:
> 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,

Since I non-posted this patch there were only two mails by you that I
saw. Prior to that we were discussing PackableStream, not this.

One mostly about the side topic of casting, with a comment about eof()
duplicate test. I found that eof earlier and removed it already before
the patch re-post.

The other was also a mix of discussion and audit, but I copied the audit
relevant part to the second post in this thread, and responded to your
points in there.

I'm not aware of anything else until this mail came back.


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

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.

More on that a bit below.

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

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.


"Old" I can agree to loose. Though it was intentional to hint for us not
to use it when talking to a cachemgr.cgi or other tool specifically
requesting a less ambigious syntax text/plain report.


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

I am hoping we can define a new non-ambiguous text syntax/format and use
that as PlainText one. If not the class name is easily changed later.

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


As to the naming;

- "page".  Well, it make no sense when the whole mgr output is a
YAML/JSON data file that get *used* by some almost completely unrelated
page 3 software layers away on a remote machine. ie MGR_INDEX script is
the page everything else is report data feeding it.


- "report" is closer. But to me is still the final output. Not the
way/syntax its produced in.

Report*er* perhapse. Then we have ReportFormatter and its children
OldCgiReporter, TextReporter, YamlReporter, JsonReporter, XmlReporter,
Snmp3Reporter, ...


Or just "Formatter" as in Mgr::Formatter and FooFormatter children



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

It was just to a) reduce the patch size, and b) skip adding
PackableStream. In the end the Stream buffer is using append() Packable
API anyway.

You want to go back to PackableStream now?

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


> 
> 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. Though its not much of one as you
argued in the earlier posts.

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

Yes.

I've left that part of the change out of this patch iteration until the
above points about it are agreed.


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


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


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

Actually I found the reverse. Started with strlen() inside the function
and ended up pushing it out to avoid unnecessary include dependencies
and pre-known length issues.

SBuf conversion made it go away.


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

Done.

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

The example uses are for Action names, but that is just an artifact of
the code so far.

The {name,len} are a generic noun section. It may be for some action
(not Action) like "PUT is not implemented", or "sdch encoding is not
implemented", or report sub-sections like "xmalloc statistics is not
implemented".


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

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

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

That is just me attempting to make the syntax a little less ambiguous.
If \n is used the notice is treated as several instead of one. Which is
nasty, but still works as it would before today.

The '\t' is imposed by the cachemgr.cgi parsing oddities for this
particular nasty format. It gets confused and treates it as a one-line
table.

Adding a filter to fix that by replacing \t with 4*SP inside notice()
instead of conflating the problem to callers.


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

"write" is incorrect. Its not writing.

"dump", well you requested for it to be renamed from dump(). And I
agree, dump implies erasure or simialr form of alteration. This is
read-only access to state data.

"display" came to mind considering that these are reports, and just as
often viewed as-is without a translation tool.


Going through the thesaurus divulgeWith() seems closest in action as
well as being commonly known.

I could also go; stage, flourish, blab, discharge,


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

Even when/if we add a non-ambiguous free-form format it would be better
to fit in between these two layers, than to inherit from this one.

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.

Amos

-------------- next part --------------
=== modified file 'src/ipc/Coordinator.cc'
--- src/ipc/Coordinator.cc	2015-01-13 07:25:36 +0000
+++ src/ipc/Coordinator.cc	2015-08-10 04:53:39 +0000
@@ -1,39 +1,40 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 /* DEBUG: section 54    Interprocess Communication */
 
 #include "squid.h"
 #include "base/Subscription.h"
 #include "base/TextException.h"
 #include "CacheManager.h"
 #include "comm.h"
 #include "comm/Connection.h"
 #include "ipc/Coordinator.h"
 #include "ipc/SharedListen.h"
 #include "mgr/Inquirer.h"
+#include "mgr/PayloadFormatter.h"
 #include "mgr/Request.h"
 #include "mgr/Response.h"
 #include "tools.h"
 #if SQUID_SNMP
 #include "snmp/Inquirer.h"
 #include "snmp/Request.h"
 #include "snmp/Response.h"
 #endif
 
 #include <cerrno>
 
 CBDATA_NAMESPACED_CLASS_INIT(Ipc, Coordinator);
 Ipc::Coordinator* Ipc::Coordinator::TheInstance = NULL;
 
 Ipc::Coordinator::Coordinator():
     Port(Ipc::Port::CoordinatorAddr())
 {
 }
 
 void Ipc::Coordinator::start()

=== modified file 'src/mgr/Action.cc'
--- src/mgr/Action.cc	2015-01-13 07:25:36 +0000
+++ src/mgr/Action.cc	2015-08-15 02:56:13 +0000
@@ -1,54 +1,68 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 /* DEBUG: section 16    Cache Manager API */
 
 #include "squid.h"
 #include "comm/Connection.h"
 #include "HttpReply.h"
 #include "ipc/Port.h"
 #include "mgr/Action.h"
 #include "mgr/ActionCreator.h"
 #include "mgr/ActionParams.h"
 #include "mgr/ActionProfile.h"
 #include "mgr/Command.h"
+#include "mgr/PayloadOldCgi.h"
 #include "mgr/Request.h"
 #include "mgr/Response.h"
 #include "SquidTime.h"
 #include "Store.h"
 
 Mgr::Action::Action(const Command::Pointer &aCmd): cmd(aCmd)
 {
     Must(cmd != NULL);
     Must(cmd->profile != NULL);
 }
 
 Mgr::Action::~Action()
 {
 }
 
+void
+Mgr::Action::dump(StoreEntry *e)
+{
+    Mgr::PayloadOldCgi p(*e);
+    displayWith(p);
+}
+
+void
+Mgr::Action::displayWith(Mgr::PayloadFormatter &p)
+{
+    p.notImplemented(SBuf(name()));
+}
+
 const Mgr::Command &
 Mgr::Action::command() const
 {
     Must(cmd != NULL);
     return *cmd;
 }
 
 bool
 Mgr::Action::atomic() const
 {
     return command().profile->isAtomic;
 }
 
 const char*
 Mgr::Action::name() const
 {
     return command().profile->name;
 }
 
 StoreEntry*

=== modified file 'src/mgr/Action.h'
--- src/mgr/Action.h	2015-01-13 07:25:36 +0000
+++ src/mgr/Action.h	2015-08-15 02:55:33 +0000
@@ -1,35 +1,36 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 /* DEBUG: section 16    Cache Manager API */
 
 #ifndef SQUID_MGR_ACTION_H
 #define SQUID_MGR_ACTION_H
 
 #include "ipc/forward.h"
 #include "mgr/forward.h"
+#include "mgr/PayloadFormatter.h"
 
 class StoreEntry;
 
 namespace Mgr
 {
 
 /// 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 Action: public RefCountable
 {
 public:
     typedef RefCount<Action> Pointer;
 
 public:
     Action(const CommandPointer &aCmd);
     virtual ~Action();
 
     /* for local Cache Manager use */
 
     /// collect + fillEntry: collect local information and fill the store entry
@@ -63,34 +64,38 @@
     /// combined data should be written at the end of the coordinated response
     virtual bool aggregatable() const { return true; } // most kid classes are
 
     bool atomic() const; ///< dump() call writes everything before returning
     const char *name() const; ///< label as seen in the cache manager menu
     const Command &command() const; ///< the cause of this action
 
     StoreEntry *createStoreEntry() const; ///< creates store entry from params
 
     ///< Content-Type: header value for this report
     virtual const char *contentType() const {return "text/plain;charset=utf-8";}
 
 protected:
     /// calculate and keep local action-specific information
     virtual void collect() {}
 
     /** start writing action-specific info to Store entry;
      * may collect info during dump, especially if collect() did nothing
      * non-atomic() actions may continue writing asynchronously after returning
      */
-    virtual void dump(StoreEntry *) {}
+    virtual void displayWith(Mgr::PayloadFormatter &);
+
+    // deprecated wrapper for displayWith().
+    // old code will override this instead of the new displayWith() method.
+    virtual void dump(StoreEntry *e);
 
 private:
     const CommandPointer cmd; ///< the command that caused this action
 
 private:
     Action(const Action &); // not implemented
     Action &operator= (const Action &); // not implemented
 };
 
 } // namespace Mgr
 
 #endif /* SQUID_MGR_ACTION_H */
 

=== modified file 'src/mgr/BasicActions.cc'
--- src/mgr/BasicActions.cc	2015-01-13 07:25:36 +0000
+++ src/mgr/BasicActions.cc	2015-08-10 05:38:50 +0000
@@ -14,41 +14,41 @@
 #include "mgr/ActionCreator.h"
 #include "mgr/ActionProfile.h"
 #include "mgr/BasicActions.h"
 #include "mgr/Registration.h"
 #include "protos.h"
 #include "SquidConfig.h"
 #include "Store.h"
 
 Mgr::IndexAction::Pointer
 Mgr::IndexAction::Create(const Command::Pointer &cmd)
 {
     return new IndexAction(cmd);
 }
 
 Mgr::IndexAction::IndexAction(const Command::Pointer &aCmd): Action(aCmd)
 {
     debugs(16, 5, HERE);
 }
 
 void
-Mgr::IndexAction::dump(StoreEntry *)
+Mgr::IndexAction::displayWith(Mgr::PayloadFormatter &)
 {
     debugs(16, 5, HERE);
 }
 
 Mgr::MenuAction::Pointer
 Mgr::MenuAction::Create(const Command::Pointer &cmd)
 {
     return new MenuAction(cmd);
 }
 
 Mgr::MenuAction::MenuAction(const Command::Pointer &aCmd): Action(aCmd)
 {
     debugs(16, 5, HERE);
 }
 
 void
 Mgr::MenuAction::dump(StoreEntry* entry)
 {
     debugs(16, 5, HERE);
     Must(entry != NULL);
@@ -58,102 +58,104 @@
 
     for (Iterator a = menu.begin(); a != menu.end(); ++a) {
         storeAppendPrintf(entry, " %-22s\t%-32s\t%s\n",
                           (*a)->name, (*a)->desc,
                           CacheManager::GetInstance()->ActionProtection(*a));
     }
 }
 
 Mgr::ShutdownAction::Pointer
 Mgr::ShutdownAction::Create(const Command::Pointer &cmd)
 {
     return new ShutdownAction(cmd);
 }
 
 Mgr::ShutdownAction::ShutdownAction(const Command::Pointer &aCmd): Action(aCmd)
 {
     debugs(16, 5, HERE);
 }
 
 void
-Mgr::ShutdownAction::dump(StoreEntry *)
+Mgr::ShutdownAction::displayWith(Mgr::PayloadFormatter &)
 {
     debugs(16, DBG_CRITICAL, "Shutdown by Cache Manager command.");
     shut_down(SIGTERM);
 }
 
 Mgr::ReconfigureAction::Pointer
 Mgr::ReconfigureAction::Create(const Command::Pointer &cmd)
 {
     return new ReconfigureAction(cmd);
 }
 
 Mgr::ReconfigureAction::ReconfigureAction(const Command::Pointer &aCmd):
     Action(aCmd)
 {
     debugs(16, 5, HERE);
 }
 
 void
-Mgr::ReconfigureAction::dump(StoreEntry* entry)
+Mgr::ReconfigureAction::displayWith(Mgr::PayloadFormatter &p)
 {
     debugs(16, DBG_IMPORTANT, "Reconfigure by Cache Manager command.");
-    storeAppendPrintf(entry, "Reconfiguring Squid Process ....");
+    static const SBuf s("Reconfiguring Squid Process ....");
+    p.notice(s);
     reconfigure(SIGHUP);
 }
 
 Mgr::RotateAction::Pointer
 Mgr::RotateAction::Create(const Command::Pointer &cmd)
 {
     return new RotateAction(cmd);
 }
 
 Mgr::RotateAction::RotateAction(const Command::Pointer &aCmd): Action(aCmd)
 {
     debugs(16, 5, HERE);
 }
 
 void
-Mgr::RotateAction::dump(StoreEntry* entry)
+Mgr::RotateAction::displayWith(Mgr::PayloadFormatter &p)
 {
     debugs(16, DBG_IMPORTANT, "Rotate Logs by Cache Manager command.");
-    storeAppendPrintf(entry, "Rotating Squid Process Logs ....");
+    static const SBuf s("Rotating Squid Process Logs ....");
+    p.notice(s);
 #if defined(_SQUID_LINUX_THREADS_)
     rotate_logs(SIGQUIT);
 #else
     rotate_logs(SIGUSR1);
 #endif
 }
 
 Mgr::OfflineToggleAction::Pointer
 Mgr::OfflineToggleAction::Create(const Command::Pointer &cmd)
 {
     return new OfflineToggleAction(cmd);
 }
 
 Mgr::OfflineToggleAction::OfflineToggleAction(const Command::Pointer &aCmd):
     Action(aCmd)
 {
     debugs(16, 5, HERE);
 }
 
 void
-Mgr::OfflineToggleAction::dump(StoreEntry* entry)
+Mgr::OfflineToggleAction::displayWith(Mgr::PayloadFormatter &p)
 {
     Config.onoff.offline = !Config.onoff.offline;
-    debugs(16, DBG_IMPORTANT, "offline_mode now " << (Config.onoff.offline ? "ON" : "OFF") << " by Cache Manager request.");
-
-    storeAppendPrintf(entry, "offline_mode is now %s\n",
-                      Config.onoff.offline ? "ON" : "OFF");
+    static const SBuf offline("offline_mode is now OFF");
+    static const SBuf online("offline_mode is now ON");
+    debugs(16, DBG_IMPORTANT, (Config.onoff.offline ? online : offline ) << " by Cache Manager request.");
+    p.notice(Config.onoff.offline ? online : offline);
 }
 
 void
 Mgr::RegisterBasics()
 {
     RegisterAction("index", "Cache Manager Interface", &Mgr::IndexAction::Create, 0, 1);
     RegisterAction("menu", "Cache Manager Menu", &Mgr::MenuAction::Create, 0, 1);
     RegisterAction("offline_toggle", "Toggle offline_mode setting", &Mgr::OfflineToggleAction::Create, 1, 1);
     RegisterAction("shutdown", "Shut Down the Squid Process", &Mgr::ShutdownAction::Create, 1, 1);
     RegisterAction("reconfigure", "Reconfigure Squid", &Mgr::ReconfigureAction::Create, 1, 1);
     RegisterAction("rotate", "Rotate Squid Logs", &Mgr::RotateAction::Create, 1, 1);
 }
 

=== modified file 'src/mgr/BasicActions.h'
--- src/mgr/BasicActions.h	2015-01-13 07:25:36 +0000
+++ src/mgr/BasicActions.h	2015-08-10 03:34:14 +0000
@@ -10,93 +10,93 @@
 
 #ifndef SQUID_MGR_BASIC_ACTIONS_H
 #define SQUID_MGR_BASIC_ACTIONS_H
 
 #include "mgr/Action.h"
 
 /* a collection of simple, mostly stateless actions */
 
 namespace Mgr
 {
 
 /// A dummy action placeholder for the no-action requests
 /// a templated Cache Manager index ('home') page.
 /// Display output is produced directly by the receiving worker
 /// without invoking the co-ordinator or action Job.
 class IndexAction: public Action
 {
 public:
     static Pointer Create(const CommandPointer &cmd);
     /* Action API */
-    virtual void dump(StoreEntry *entry);
+    virtual void displayWith(Mgr::PayloadFormatter &) override final;
 
 protected:
     IndexAction(const CommandPointer &cmd);
 };
 
 /// returns available Cache Manager actions and their access requirements
 class MenuAction: public Action
 {
 public:
     static Pointer Create(const CommandPointer &cmd);
     /* Action API */
     virtual void dump(StoreEntry *entry);
 
 protected:
     MenuAction(const CommandPointer &cmd);
 };
 
 /// shuts Squid down
 class ShutdownAction: public Action
 {
 public:
     static Pointer Create(const CommandPointer &cmd);
     /* Action API */
-    virtual void dump(StoreEntry *entry);
+    virtual void displayWith(Mgr::PayloadFormatter &) override final;
 
 protected:
     ShutdownAction(const CommandPointer &cmd);
 };
 
 /// reconfigures Squid
 class ReconfigureAction: public Action
 {
 public:
     static Pointer Create(const CommandPointer &cmd);
     /* Action API */
-    virtual void dump(StoreEntry *entry);
+    virtual void displayWith(Mgr::PayloadFormatter &) override final;
 
 protected:
     ReconfigureAction(const CommandPointer &cmd);
 };
 
 /// starts log rotation
 class RotateAction: public Action
 {
 public:
     static Pointer Create(const CommandPointer &cmd);
     /* Action API */
-    virtual void dump(StoreEntry *entry);
+    virtual void displayWith(Mgr::PayloadFormatter &) override final;
 
 protected:
     RotateAction(const CommandPointer &cmd);
 };
 
 /// changes offline mode
 class OfflineToggleAction: public Action
 {
 public:
     static Pointer Create(const CommandPointer &cmd);
     /* Action API */
-    virtual void dump(StoreEntry *entry);
+    virtual void displayWith(Mgr::PayloadFormatter &) override final;
 
 protected:
     OfflineToggleAction(const CommandPointer &cmd);
 };
 
 /// Registeres profiles for the actions above; \todo move elsewhere?
 void RegisterBasics();
 
 } // namespace Mgr
 
 #endif /* SQUID_MGR_BASIC_ACTIONS_H */
 

=== modified file 'src/mgr/Makefile.am'
--- src/mgr/Makefile.am	2015-01-13 07:25:36 +0000
+++ src/mgr/Makefile.am	2015-08-15 02:17:24 +0000
@@ -25,39 +25,43 @@
 	BasicActions.h \
 	Command.cc \
 	Command.h \
 	CountersAction.cc \
 	CountersAction.h \
 	Filler.cc \
 	Filler.h \
 	Forwarder.cc \
 	Forwarder.h \
 	forward.h \
 	FunAction.cc \
 	FunAction.h \
 	InfoAction.cc \
 	InfoAction.h \
 	Inquirer.cc \
 	Inquirer.h \
 	IntervalAction.cc \
 	IntervalAction.h \
 	IoAction.cc \
 	IoAction.h \
+	PayloadFormatter.cc \
+	PayloadFormatter.h \
+	PayloadOldCgi.cc \
+	PayloadOldCgi.h \
 	Registration.cc \
 	Registration.h \
 	Request.cc \
 	Request.h \
 	Response.cc \
 	Response.h \
 	ServiceTimesAction.cc \
 	ServiceTimesAction.h \
 	StoreIoAction.cc \
 	StoreIoAction.h \
 	StoreToCommWriter.cc \
 	StoreToCommWriter.h \
 	QueryParam.h \
 	QueryParams.cc \
 	QueryParams.h \
 	IntParam.cc \
 	IntParam.h \
 	StringParam.cc \
 	StringParam.h

=== added file 'src/mgr/PayloadFormatter.cc'
--- src/mgr/PayloadFormatter.cc	1970-01-01 00:00:00 +0000
+++ src/mgr/PayloadFormatter.cc	2015-08-15 06:19:45 +0000
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "mgr/PayloadFormatter.h"
+#include "SBuf.h"
+
+Mgr::PayloadFormatter::PayloadFormatter(Packable &p) :
+       outBuf(p)
+{
+}
+
+void
+Mgr::PayloadFormatter::notImplemented(const SBuf &s)
+{
+    SBuf note(s);
+    note.append(" is not implemented",19);
+    notice(note);
+}
+

=== added file 'src/mgr/PayloadFormatter.h'
--- src/mgr/PayloadFormatter.h	1970-01-01 00:00:00 +0000
+++ src/mgr/PayloadFormatter.h	2015-08-15 05:20:40 +0000
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+/* DEBUG: section 16    Cache Manager API */
+
+#ifndef SQUID_SRC_MGR_PAYLOADFORMATTER_H
+#define SQUID_SRC_MGR_PAYLOADFORMATTER_H
+
+#include "base/Packable.h"
+#include "mgr/forward.h"
+
+class SBuf;
+
+namespace Mgr
+{
+
+/// Base API for formatting the response payload for a cache manager command.
+class PayloadFormatter
+{
+public:
+    explicit PayloadFormatter(Packable &);
+    virtual ~PayloadFormatter() = default;
+
+    /// output a '... is Not Implemented' notice
+    void notImplemented(const SBuf &);
+
+    // a comment
+    virtual void notice(const SBuf &) = 0;
+
+protected:
+    Packable &outBuf;
+};
+
+} // namepace Mgr
+
+#endif /* SQUID_SRC_MGR_PAYLOADFORMATTER_H */

=== added file 'src/mgr/PayloadOldCgi.cc'
--- src/mgr/PayloadOldCgi.cc	1970-01-01 00:00:00 +0000
+++ src/mgr/PayloadOldCgi.cc	2015-08-15 05:45:06 +0000
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+/* DEBUG: section 16    Cache Manager API */
+
+#include "squid.h"
+#include "base/CharacterSet.h"
+#include "base/Packable.h"
+#include "mgr/PayloadOldCgi.h"
+#include "SBuf.h"
+
+void
+Mgr::PayloadOldCgi::notice(const SBuf &s)
+{
+    // cachemgr.cgi cannot cope with \t inside notice text
+    // it can kind of cope with \n, so ignore that for now.
+    // but we may need to add it for other tools later.
+    static const CharacterSet tab("TAB", "\t");
+
+    SBuf tmp(s);
+    while (tmp.length()) {
+        auto end = tmp.findFirstOf(tab);
+        SBuf t = tmp.consume(end);
+        outBuf.append(t.rawContent(), t.length());
+
+        if (end != SBuf::npos) {
+            outBuf.append("    ",4);
+            tmp.consume(1);
+        }
+    }
+
+    outBuf.append("\n",1);
+}
+

=== added file 'src/mgr/PayloadOldCgi.h'
--- src/mgr/PayloadOldCgi.h	1970-01-01 00:00:00 +0000
+++ src/mgr/PayloadOldCgi.h	2015-08-12 11:07:37 +0000
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+/* DEBUG: section 16    Cache Manager API */
+
+#ifndef SQUID_SRC_MGR_PAYLOADOLDCGI_H
+#define SQUID_SRC_MGR_PAYLOADOLDCGI_H
+
+#include "mgr/PayloadFormatter.h"
+
+namespace Mgr
+{
+
+/**
+ * Produces cache manager report payloads in a human-readable markup syntax
+ * which is parsed by the cachemgr.cgi tool.
+ *
+ * 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)
+ */
+class PayloadOldCgi: public PayloadFormatter
+{
+public:
+    PayloadOldCgi(Packable &p): PayloadFormatter(p) {}
+    virtual ~PayloadOldCgi() = default;
+
+    /* PayloadFormatter API */
+    virtual void notice(const SBuf &) override final;
+};
+
+} // namepace Mgr
+
+#endif /* SQUID_SRC_MGR_PAYLOADOLDCGI_H */

=== modified file 'src/mgr/forward.h'
--- src/mgr/forward.h	2015-08-01 02:13:13 +0000
+++ src/mgr/forward.h	2015-08-10 05:06:30 +0000
@@ -6,40 +6,42 @@
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 /* DEBUG: section 16    Cache Manager API */
 
 #ifndef SQUID_MGR_FORWARD_H
 #define SQUID_MGR_FORWARD_H
 
 #include "base/RefCount.h"
 
 /// Cache Manager API
 namespace Mgr
 {
 
 class Action;
 class ActionCreator;
 class ActionPasswordList;
 class ActionProfile;
 class ActionWriter;
 class Command;
+class PayloadFormatter;
+class PayloadOldCgi;
 class Request;
 class Response;
 class QueryParam;
 class QueryParams;
 
 typedef RefCount<Action> ActionPointer;
 typedef RefCount<ActionProfile> ActionProfilePointer;
 typedef RefCount<ActionCreator> ActionCreatorPointer;
 typedef RefCount<Command> CommandPointer;
 
 typedef ActionPointer (ClassActionCreationHandler)(const CommandPointer &cmd);
 
 } // namespace Mgr
 
 class StoreEntry;
 /**
  * Handler for "dumping" out a cachemgr report to a StoreEntry
  */
 typedef void OBJH(StoreEntry *);
 

=== modified file 'src/tests/stub_libmgr.cc'
--- src/tests/stub_libmgr.cc	2015-01-13 07:25:36 +0000
+++ src/tests/stub_libmgr.cc	2015-08-10 13:21:50 +0000
@@ -15,76 +15,77 @@
 // NP: used by Command.h instantiations
 #include "mgr/ActionProfile.h"
 
 // NP: used by Action.h instantiations
 #include "mgr/Command.h"
 std::ostream &operator <<(std::ostream &os, const Mgr::Command &cmd) STUB_RETVAL(os)
 
 #include "mgr/Action.h"
 Mgr::Action::Action(const CommandPointer &aCmd) STUB
 Mgr::Action::~Action() STUB
 void Mgr::Action::run(StoreEntry *entry, bool writeHttpHeader) STUB
 void Mgr::Action::fillEntry(StoreEntry *entry, bool writeHttpHeader) STUB
 void Mgr::Action::add(const Action &action) STUB
 void Mgr::Action::respond(const Request &request) STUB
 void Mgr::Action::sendResponse(unsigned int requestId) STUB
 bool Mgr::Action::atomic() const STUB_RETVAL(false)
 const char * Mgr::Action::name() const STUB_RETVAL(NULL)
 static Mgr::Command static_Command;
 const Mgr::Command & Mgr::Action::command() const STUB_RETVAL(static_Command)
 StoreEntry * Mgr::Action::createStoreEntry() const STUB_RETVAL(NULL)
+void Mgr::Action::dump(StoreEntry *) STUB
 static Mgr::Action::Pointer dummyAction;
 
 #include "mgr/ActionParams.h"
 Mgr::ActionParams::ActionParams() STUB_NOP
 Mgr::ActionParams::ActionParams(const Ipc::TypedMsgHdr &msg) STUB_NOP
 void Mgr::ActionParams::pack(Ipc::TypedMsgHdr &msg) const STUB
 std::ostream &operator <<(std::ostream &os, const Mgr::ActionParams &params) STUB_RETVAL(os)
 
 #include "mgr/ActionWriter.h"
 //Mgr::ActionWriter::ActionWriter(const Action::Pointer &anAction, int aFd) STUB
 //protected:
 void Mgr::ActionWriter::start() STUB
 
 #include "mgr/BasicActions.h"
 Mgr::Action::Pointer Mgr::MenuAction::Create(const Mgr::CommandPointer &cmd) STUB_RETVAL(dummyAction)
 void Mgr::MenuAction::dump(StoreEntry *entry) STUB
 //protected:
 //Mgr::MenuAction::MenuAction(const CommandPointer &cmd) STUB
 
 Mgr::Action::Pointer Mgr::ShutdownAction::Create(const Mgr::CommandPointer &cmd) STUB_RETVAL(dummyAction)
-void Mgr::ShutdownAction::dump(StoreEntry *entry) STUB
+void Mgr::ShutdownAction::displayWith(Mgr::PayloadFormatter &) STUB
 // protected:
 //Mgr::ShutdownAction::ShutdownAction(const CommandPointer &cmd) STUB
 
 Mgr::Action::Pointer Mgr::ReconfigureAction::Create(const Mgr::CommandPointer &cmd) STUB_RETVAL(dummyAction)
-void Mgr::ReconfigureAction::dump(StoreEntry *entry) STUB
+void Mgr::ReconfigureAction::displayWith(Mgr::PayloadFormatter &) STUB
 //protected:
 //Mgr::ReconfigureAction::ReconfigureAction(const CommandPointer &cmd) STUB
 
 Mgr::Action::Pointer Mgr::RotateAction::Create(const Mgr::CommandPointer &cmd) STUB_RETVAL(dummyAction)
-void Mgr::RotateAction::dump(StoreEntry *entry) STUB
+void Mgr::RotateAction::displayWith(Mgr::PayloadFormatter &) STUB
 //protected:
 //Mgr::RotateAction::RotateAction(const CommandPointer &cmd) STUB
 
 Mgr::Action::Pointer Mgr::OfflineToggleAction::Create(const CommandPointer &cmd) STUB_RETVAL(dummyAction)
-void Mgr::OfflineToggleAction::dump(StoreEntry *entry) STUB
+void Mgr::OfflineToggleAction::displayWith(Mgr::PayloadFormatter &) STUB
 //protected:
 //Mgr::OfflineToggleAction::OfflineToggleAction(const CommandPointer &cmd) STUB
 
 void Mgr::RegisterBasics() STUB
 
 #include "mgr/CountersAction.h"
 //Mgr::CountersActionData::CountersActionData() STUB
 Mgr::CountersActionData& Mgr::CountersActionData::operator +=(const Mgr::CountersActionData& stats) STUB_RETVAL(*this)
 
 Mgr::Action::Pointer Mgr::CountersAction::Create(const CommandPointer &cmd) STUB_RETVAL(dummyAction)
 void Mgr::CountersAction::add(const Action& action) STUB
 void Mgr::CountersAction::pack(Ipc::TypedMsgHdr& msg) const STUB
 void Mgr::CountersAction::unpack(const Ipc::TypedMsgHdr& msg) STUB
 //protected:
 //Mgr::CountersAction::CountersAction(const CommandPointer &cmd) STUB
 void Mgr::CountersAction::collect() STUB
 void Mgr::CountersAction::dump(StoreEntry* entry) STUB
 
 #include "mgr/Filler.h"
 //Mgr::Filler::Filler(const Action::Pointer &anAction, int aFd, unsigned int aRequestId) STUB



More information about the squid-dev mailing list