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. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev