Re: [MERGE] Initial netfilter mark patch for comment

Thanks for the prompt response. Updated patch attached to my previous
email.

> > 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.
>
> 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.

Release notes for 3.2 updated.

> 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.

Fixed.

> * 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.

Fixed. I've not looked at AC_CHECK_CACHED yet; I can do so, although it
doesn't appear to be used anywhere else in configure.in.

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

Fixed.

> 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+.

Most warnings have been changed to 2, except where a higher level is
warranted.

> 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.

Fixed.

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

Fixed.

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

Fixed.

> * 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.

The #defs have all been updated in accordance with my previous email.

> * 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");
>

I tried removing the GetAddrInfo(), but that doesn't work as it
retrieves some of the required connection info. However, I have moved
the FreeAddrInfo down as suggested, but outside of the 'if' so that it
pairs with the GetAddrInfo.

> * 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.

Fixed.

> * 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.

Fixed.

> 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.

I've used DBG_CRITICAL throughout the parseConfigLine, as nothing else
produces any output. Let me know if I should change this.

> * 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.

Let me know what you think. For the mark value, if sscanf is required,
then the inputs will have to be fixed as either 0x or decimal, as it
can't auto-detect for an unsigned integer.

Also, if we can stick with strtoul, then the return value of -1 can be
used to test for invalid values and error as appropriate.

> * 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.

Added as a DBG_CRITICAL for the reasons outlined above.

> * 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.

All fixed. Because snprintf includes the null-terminating character of a
string when calculating length, I increased the existing values for the
tos patch, otherwise it doesn't print correctly.

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

Thanks. Fixed. The dangers of copy & paste :)

> 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.

Added to the cf.data.pre. Should it be added anywhere else?

> * 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.

Both functions added. As per last email, I suggest that isTosActive() is
used instead of USE_ZPH_QOS (or USE_QOS_TOS).

Regards,

Andy
Received on Sat Aug 07 2010 - 08:11:37 MDT