Re: Patch to add netfilter mark support

> src/Parsing.*:
> * Please palce the strtou*() functions to a file under lib/ with an .h in
> include/. At this stage they get linked in globally through lib/libmisc.la.

Done, but as they both return bool I've had to add stdbool.h to the
headers that the file includes and also add a check for stdbool.h to
configure.in. Should I stick with this methodology or is it better that
I just change the return type to an int?

> * Can you also mention in the function docs the overflow reason for
> (re-)defining our own wrappers.

Done.

> src/cache_cf.cc:
> * You include <limits>. This needs to be wrapped in #if HAVE_LIMITS and
> listed with the other system file includes, not the squid internal
> includes.

Done.

> - If any squid .h requires it to be defined first then it MUST be
> included directly by that .h to pass dependency checks.
>
> * "Changed from sscanf for greater flexibility for inputs and error
> checking" comment is not needed.

Removed.

> * Config.accessList.outgoingTos, Config.accessList.clientsideTos,
> Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark can
> become members of the Qos scope Config object. All the parsing /free stuff
> can be moved there too with some #define parse_...() etc for the legacy
> parser.

I agree that this would be desirable, but I'm not sure how to do it
without things getting messy. Because each of the parameters applies to
an access list and not globally, surely they need to stay within the
accessList struct, which means I can't move them to the Qos class?

I guess I can still move the parsing stuff to Qos though.

> Along with Alex comment about protos, I think this unwraps any need of the
> QoS stuff to include structs.h,protos.h and typedefs.h
>
> cf.data.pre:
> * the old docs text about the above ACL tests being "based on the
> username or source address making the request" is probably quite wrong.
> It's based on ANY request/connection criteria is it not? and possibly reply
> details as well for the reply packets.

Changed to "based on an ACL."

> * please also use CIDR masks in the new documentation. class masks should
> have been killed long ago.

Done.

> src/ip/QosConfig.h:
> * please wrap the system includes separately, with a check for each in
> configure.in.

Done.

Thanks,

Andy
Received on Sun Sep 12 2010 - 23:07:08 MDT