Re: [MERGE] Initial netfilter mark patch for comment

On Mon, 02 Aug 2010 00:47:15 +0100, Andrew Beverley <andy_at_andybev.com>
wrote:
> Please find attached the first version of the netfilter mark patch. I've
> not yet tested it extensively, but would welcome some initial feedback
> or comments.
>
> My comments are:
>
> - The existing TOS patch cannot be disabled at runtime. As such, this
> mark patch cannot be either. Would it be preferable to only enable them
> both if the qos_flows config option is present? This would also have the
> advantage of being able to add CAP_NET_ADMIN as appropriate at runtime.
>
> - I have used sscanf instead of strtoul for both TOS and MARK in
> QosConfig.cc (sscanf doesn't auto-detect the format of unsigned long
> int). As a result, the tos variable could be changed to type char, which
> is what it should be in my opinion. Should I do this?
>
> - The code in forward.cc to obtain all the data needed to locate the
> connection information is messy. Is there a better way to achieve it?
> Conntrack needs to be passed local and remote port numbers and IP
> addresses.
>

The mess around local port can be cleaned up (see comments below). The
remote ip:port mess seems as clean as it can be in the current code.

>
> Is it too late to get this in v3.2? I will update the appropriate
> release-notes once I know.

Not too late. This can come under a header of polish on the existing QoS
feature. We have a a bit more flexibility on that kind of change.

The changes to qos_flows needs to be mentioned in section 3.2 "changes to
existing tags" and the --with option to section 4.2 "new ./configure
options". Both are alphabetical lists.

Audit Results:

configure.in: chunk line 2087
 * "QOS netfilter marking enabled: $with_netfilter_conntrack" if ZPH QoS
is enabled, but regardless of whether NFMARK was defined.
  The notice needs to be inside the top part of your inner if statement.

 * Please also add an
AC_CHECK_HEADERS([libnetfilter_conntrack/libnetfilter_conntrack.h
libnetfilter_conntrack/libnetfilter_conntrack_tcp.h])
 That way you can error-out early in the ./configure build process with a
useful AC_MSG_ERROR() and also wrap the headers in forward.cc according to
squid coding style.
 NP: this check is also one that can be cached AC_CHECK_CACHED() I think.

src/cf.data.pre typo:
"in the case of TOS preservation require your kernel to be patch with"
 ==> s /patch/patched/

src/comm.cc:
 * Please use DBG_IMPORTANT instead of '1' and '0' in debugs. Also, IMO
both should indicate "WARNING:", unless the one currently logged at level 1
is common and trivial, in which case it should be dropped to level 2+.

The CRITICAL level as much as possible should be restricted to "ERROR:"
(serious but recoverable or only affecting this users request) and "FATAL:"
(non-recoverable, Squid about to shutdown).
The IMPORTANT level should be "WARNING:" and similar. Stuff that can be
administratively fixed etc.

src/forward.cc:
 * Wrap system includes in "#if HAVE_*"
  ** also they appear to be duplicate included in forward.h and
forward.cc, please only include in the forward.h.

 * Please remove the "defined(_SQUID_LINUX_)" around getNfctMark. We build
on non-linux systems with possible netfilter support (kFreeBSD and hurd).

 * our source format requires function result type to be on a separate
line in .cc:
 +int
 +FwdState::getNfctMark

 * also related to the above. The old ZPH TOS _SQUID_LINUX_ wrapper was
only there due to the preserve-miss patch being linux kernel specific. I
think your new preserve for marks not depending on kernel patching is
likely not to need it, so the wrapping there can be re-worked a bit to only
depend on USE_ZPH_QOS and USE_QOS_NFMARK. If you make this change the docs
will also need updating.

 * You run the sequence:
+ serv_fde_local_conn.InitAddrInfo(addr);
+ serv_fde_local_conn.FreeAddrInfo(addr);
+ serv_fde_local_conn.GetAddrInfo(addr);

 ** The GetAddrInfo allocates dynamic memory and needs to be paired with a
FreeAddrInfo() of its own.
 ** The structures created locally by Init inside addr should be able to
be used throughout the sequence. Unless there is some garbage produced by
getaddrinfo() please remove the Get and move the Free down below where you
are finished with the addr variable and it's content:

+ serv_fde_local_conn.FreeAddrInfo(addr);
+ } else {
+ debugs(17, 1, "Failed to allocate new conntrack");

 *

 * following nfct_query() you can output serv_fde_local_conn directly in
the debugs() argument instead of fiddling with inet_ntop to find its
content. That will handle all the display mess and output src ip:port as a
twinned pair. If the debugs result shows no port it means there was none
set.

 * again with the debugs level s/1/DBG_IMPORTANT/ and tagged "WARNING:" if
it's an error important message. Or bump to level 2+ if its not.

src/ip/QosConfig.cc:
 * Ip::Qos::QosConfig::parseConfigLine again with the s/0/DBG_CRITICAL/.
This one I think will screw up TOS settings if it continues so a "FATAL:"
message and self_destruct() are warranted when mark configuration is
attempted.

 * Thanks for knowing about strtoul(). We had some issues with portability
and the optional-prefixes last time we tried to get away from sscanf (to
strtol IIRC). We shall have to look into this one a bit more about security
and string validation, termination issues etc but it certainly passes the
input syntax hurdles.

 * on "miss-mask=" there "else" case missing in "if (mark ... else if ...
". This needs to output some level-1 WARNING: about the preserve-miss not
being set before the mask.

 * dumpConfigLine() is used to generate a copy of the configuration file.
Sometimes this output is used for upgrades. Your code needs to re-create
the correct preferred configuration syntax for the currently running
settings.
  For this I think duplicating the existing function content entirely to
generate lines prefixxed with "qos_flags tos" and "qos_flags mark" if any
of their related settings are configured.
  I see a bug in the existing code, it can produce an empty "qos_flags\n"
line when no TOS flags are set. If you can fix that while wrapping the
"qos_flags tos" version of the line it would be great.

 * you are also currently outputting tos_* values in the dumps labeled
"(mark)"

src/tools.cc:
 * Since CAP_NET_ADMIN is needed, your docs also need to mention that
libcap version 2.09+ is required to use this feature. Related to the
--with-libcap option.

 * The capabilities are set post-configuration. So you have access to
Ip::QosConfig.* to determine whether to set CAP_NET_ADMIN. This will let
you use an "if...else if" instead of #if for selective enabling.
  You may find isMarkActive()/isTosActive() inline test methods added to
QosConfig useful for both this and the dump procedure above.

Amos
Received on Mon Aug 02 2010 - 02:33:29 MDT