Re: Patch to add netfilter mark support

> * 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
Received on Tue Sep 07 2010 - 20:35:20 MDT