> * I find the terminology inconsistent and confusing: outgoing, > clientside, upstream. No wonder you have to explain the difference > twice. Unless these are all standard RFC-like terms, please use > something consistent like fromClient, toClient, fromServer, toServer. > Others may suggest a better scheme, but this one at least does not > require constant doc lookups to understand where "out" and "up" is.
All variables changed as suggested; squid.conf configuration parameters unchanged. > * TOS (unsigned char) and mark (uint32_t) types are repeated numerous > times throughout the patch. Do you thin it would be a good idea to > typedef them? It seems fairly easy for somebody to type them wrong or > mix them up. I cannot insist on this change, but it would be the right > thing to do, IMO. Changed to tos_t and nfmark_t > * If TOS and mark types can be the same uint32_t, we should probably use > the same type and avoid at least some of the code duplication due to the > difference in types. Kept different as per Amos' email. > * The isAclNfmarkActive() and isAclTosActive() functions appear to be > unrelated to src/fde.cc. They are about configuration, right? Why not > make them Qos globals or, of the lists are moved, Qos::Config methods? Moved to Qos::Config, although renamed the existing isMarkActive to differentiate. > * I believe Amos does not want new functions in protos.h unless > necessary. Can you declare getOutgoingTOS() and getOutgoingNfmark() in > src/forward.h? Moved to forward.h > And capitalize the first letter since they are global? No longer global; moved within FwdState. > * Did you verify that "make -k check" and "./test-builds.sh" did not > break because of your changes? No... but in progress as I write. > > * Please remove the following redundant lines (no need to document what > standard methods are): > > > + /** > > + * Constructor > > + */ > > > + /** > > + * Destructor > > + */ Done. > * s/Returns true or false depending on whether/whether/g Done. > * Not sure, but perhaps s/whether we should carry out the QOS functions > for/whether to apply QOS functions to/. This would allow you to collapse > the comment to just one line. Changed to something even better :) > * Closing paren missing in isAclNfmarkActive() description. Done. > * You can use /// comments for /** one line comments */ to make them a > little shorter and avoid line wrapping. Done. Patch to follow shortly. Thanks, Andy
