[hipl-dev] Re: [Merge] lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: HIPL core team <hipl-dev@xxxxxxxxxxxxx>
- Date: Tue, 14 Dec 2010 15:48:34 +0100
On Mon, Dec 13, 2010 at 09:29:08PM +0000, Diego Biurrun wrote:
> You have been requested to review the proposed merge of
> lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl.
>
> This is Christoph's branch updated to current trunk. I haven't reviewed it
> in detail yet, just fixed a few things I noticed right away. This still
> needs careful review and testing.
> Below is the original merge proposal text from Christoph:
Hah, I get to review code I submitted myself... :-)
> --- firewall/conntrack.c 2010-12-13 19:09:27 +0000
> +++ firewall/conntrack.c 2010-12-13 21:28:35 +0000
> @@ -73,9 +76,40 @@
>
> +#ifdef CONFIG_HIP_DEBUG
> +// this improves our chances of finding bugs in the timeout code
> +#define DEFAULT_CONNECTION_TIMEOUT 20; // 20 seconds
> +#define DEFAULT_CLEANUP_INTERVAL 10; // 10 seconds
> +#else
> +#define DEFAULT_CONNECTION_TIMEOUT (60 * 5); // 5 minutes
> +#define DEFAULT_CLEANUP_INTERVAL (60 * 60); // 1 minute
> +#endif
I'd drop this debugging #ifdef from the merge and keep it local.
> +/**
> + * Interval between sweeps in hip_fw_conntrack_periodic_cleanup(),
> + * in Seconds.
> + *
> + * @see hip_fw_conntrack_periodic_cleanup()
> + */
> +static const time_t cleanup_interval = DEFAULT_CLEANUP_INTERVAL;
> +
> +/**
> + * Connection timeout, in seconds.
> + * Disabled if zero. This actually specifies the minimum period of
> + * inactivity before a connection is considered stale.
> + * Thus, a connection may be inactive for at most @c connection_timeout
> + * plus @c cleanup_interval seconds.
> + *
> + * @see hip_fw_conntrack_periodic_cleanup()
> + */
> +time_t connection_timeout = DEFAULT_CONNECTION_TIMEOUT;
I wonder a bit where the constants come from and why they are not used
directly...
> +/**
> + * @todo: singly-linked may suffice.
Find out ;-)
> @@ -319,23 +352,147 @@
> + err = system_printf("iptables %s %s -p UDP "
> + "--dport 10500 --sport 10500 -d %s -m u32 "
> + "--u32 '4&0x1FFF=0 && 0>>22&0x3C@8=0x%08X'
> -j ACCEPT",
> + flag, table, daddr, esp_tuple->spi);
Here and in a few other places: I don't think breaking those lines helps
readability too much - then again, I don't really care...
> @@ -425,9 +584,11 @@
> * initialize and store a new HIP/ESP connnection into the connection table
> *
> * @param data the connection-related data to be inserted
> + * @param ctx the context
> * @see remove_connection
> */
> -static void insert_new_connection(const struct hip_data *data)
> +static void insert_new_connection(const struct hip_data *data,
> + const struct hip_fw_context *ctx)
The doxygen comment is a bit terse.
> + unsigned int i;
> +
> + for (i = 0; i < snap_count; ++i) {
Is there a special reason for making the loop counter unsigned?
> +/**
> + * Cache current iptables status.
> + * Currently, this works by parsing the output given by @a cmd, which
> + * is expected to have `iptables --nvL' format.
> + *
> + * @param cmd command line to capture output from
> + * @param out gathered info will be written here
> + * @param sz no more than @a sz entries will be written
> + * @return number of entries written
> + *
> + */
> +static size_t populate_snapshot(const char *cmd,
> + struct esp_rule_status *out,
> + const size_t sz)
"sz" does not sound explanatory or intuitive to me, what is it?
> + static const char u32_prefix[] = "u32
> 0x4&0x1fff=0x0&&0x0>>0x16&0x3c@0x8=0x";
That expression could use some spaces for readability, but I'm not sure
the string is under your control...
> +
> + if ((str_spi = strstr(bfr, "spi:")) != NULL) {
> + // non-udp
> + if (sscanf(str_spi, "spi:%u", &out[ret].spi) < 1) {
> + HIP_ERROR("Malformed iptables output: %s\n", bfr);
> + continue;
> + }
> + } else if ((str_spi = strstr(bfr, u32_prefix)) != NULL) {
> + // udp
> + if (sscanf(str_spi + (sizeof(u32_prefix) - 1), "%x",
> &out[ret].spi) < 1) {
> + HIP_ERROR("Malformed iptables output: %s\n", bfr);
> + continue;
The != NULL is pointless, as are the parentheses around sizeof().
Also, the expressions look similar enough to be refactored into one.
> +void hip_fw_conntrack_periodic_cleanup(void)
> +{
> + struct slist *iter_conn = NULL;
> +
> + while (iter_conn) {
> + struct connection *conn = (struct connection*) iter_conn->data;
That's a pointless void* cast that I apparently overlooked...
> --- hipd/hiprelay.c 2010-11-30 14:50:30 +0000
> +++ hipd/hiprelay.c 2010-12-13 21:28:35 +0000
> @@ -1015,16 +1015,16 @@
> *
> * @param r the HIP control message to be relayed
> * @param type_hdr message type
> - * @param r_saddr the original source address
> - * @param r_daddr the original destination address
> + * @param r_saddr (unused) the original source address
> + * @param r_daddr (unused) the original destination address
> * @param relay_to_addr the address where to relay the packet
> * @param relay_to_port the port where to relay the packet
> * @return zero on success or negative on error
> */
> static int hip_relay_forward_response(const hip_common_t *r,
> const uint8_t type_hdr,
> - const struct in6_addr *r_saddr,
> - const struct in6_addr *r_daddr,
> + DBG const struct in6_addr *r_saddr,
> + DBG const struct in6_addr *r_daddr,
> const struct in6_addr *relay_to_addr,
> const in_port_t relay_to_port)
Why not drop the unused parameters?
> --- lib/core/debug.h 2010-12-06 19:58:46 +0000
> +++ lib/core/debug.h 2010-12-13 21:28:35 +0000
> @@ -295,10 +296,17 @@
> #define HIP_INFO_LSI(str, lsi) hip_print_lsi(DEBUG_LEVEL_INFO, __FILE__,
> __LINE__, __FUNCTION__, str, lsi)
> #define HIP_INFO_INADDR(str, in) hip_print_lsi(DEBUG_LEVEL_INFO, __FILE__,
> __LINE__, __FUNCTION__, str, in)
>
> +#ifdef CONFIG_HIP_DEBUG
> #define HIP_DEBUG_HIT(str, hit) hip_print_hit(DEBUG_LEVEL_DEBUG, __FILE__,
> __LINE__, __FUNCTION__, str, hit)
> #define HIP_DEBUG_IN6ADDR(str, in6) hip_print_hit(DEBUG_LEVEL_DEBUG,
> __FILE__, __LINE__, __FUNCTION__, str, in6)
> #define HIP_DEBUG_LSI(str, lsi) hip_print_lsi(DEBUG_LEVEL_DEBUG, __FILE__,
> __LINE__, __FUNCTION__, str, lsi)
> #define HIP_DEBUG_INADDR(str, in) hip_print_lsi(DEBUG_LEVEL_DEBUG,
> __FILE__, __LINE__, __FUNCTION__, str, in)
> +#else
> +#define HIP_DEBUG_HIT(str, hit) do {} while (0)
> +#define HIP_DEBUG_IN6ADDR(str, in6) do {} while (0)
> +#define HIP_DEBUG_LSI(str, lsi) do {} while (0)
> +#define HIP_DEBUG_INADDR(str, in) do {} while (0)
> +#endif
This looks unrelated. Why?
Diego
Other related posts:
