Hi Harsha,
On Sun, Aug 05, 2018 at 02:49:22PM +0530, Harsha Sharma wrote:
[...]
> diff --git a/include/libnftnl/cttimeout.h b/include/libnftnl/cttimeout.h
> new file mode 100644
> index 0000000..6f0ef75
> --- /dev/null
> +++ b/include/libnftnl/cttimeout.h
> @@ -0,0 +1,39 @@
> +#ifndef _LIBNETFILTER_CTTIMEOUT_H_
> +#define _LIBNETFILTER_CTTIMEOUT_H_
> +
> +#include <stdint.h>
> +#include <sys/types.h>
> +#include <linux/netfilter/nfnetlink_conntrack.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct nftnl_obj_ct_timeout;
> +
> +enum nftnl_obj_ct_timeout_tcp_attr {
> + NFTA_CT_TIMEOUT_ATTR_TCP_SYN_SENT = 0,
> + NFTA_CT_TIMEOUT_ATTR_TCP_SYN_RECV,
> + NFTA_CT_TIMEOUT_ATTR_TCP_ESTABLISHED,
> + NFTA_CT_TIMEOUT_ATTR_TCP_FIN_WAIT,
> + NFTA_CT_TIMEOUT_ATTR_TCP_CLOSE_WAIT,
> + NFTA_CT_TIMEOUT_ATTR_TCP_LAST_ACK,
> + NFTA_CT_TIMEOUT_ATTR_TCP_TIME_WAIT,
> + NFTA_CT_TIMEOUT_ATTR_TCP_CLOSE,
> + NFTA_CT_TIMEOUT_ATTR_TCP_SYN_SENT2,
> + NFTA_CT_TIMEOUT_ATTR_TCP_RETRANS,
> + NFTA_CT_TIMEOUT_ATTR_TCP_UNACK,
> + NFTA_CT_TIMEOUT_ATTR_TCP_MAX
You can use enum tcp_conntrack instead, this is already part of the
uapi, so it is exposed to userspace. So we can get rid of this.
> +};
> +
> +enum nftnl_obj_ct_timeout_udp_attr {
> + NFTA_CT_TIMEOUT_ATTR_UDP_UNREPLIED = 0,
> + NFTA_CT_TIMEOUT_ATTR_UDP_REPLIED,
> + NFTA_CT_TIMEOUT_ATTR_UDP_MAX
> +};
I would like you kill this libnftnl/cttimeout.h file and the
nftnl_timeout_policy_attr_set_u32() interface.
My proposal is: Pass an array of int to NFTNL_OBJ_CT_TIMEOUT_POLICY
via nftnl_obj_set_data(), where -1 means unset, ie. timeout for this
state is not set and default existing value should be used instead.
More comments below.
[...]
> diff --git a/include/libnftnl/object.h b/include/libnftnl/object.h
> index 93a40d0..14f6e83 100644
> --- a/include/libnftnl/object.h
> +++ b/include/libnftnl/object.h
> @@ -7,6 +7,7 @@
> #include <sys/types.h>
>
> #include <libnftnl/common.h>
> +#include <libnftnl/cttimeout.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -41,6 +42,13 @@ enum {
> NFTNL_OBJ_CT_HELPER_L4PROTO,
> };
>
> +enum {
> + NFTNL_OBJ_CT_TIMEOUT_L3PROTO = NFTNL_OBJ_BASE,
> + NFTNL_OBJ_CT_TIMEOUT_L4PROTO,
> + NFTNL_OBJ_CT_TIMEOUT_DATA,
> + NFTNL_OBJ_CT_TIMEOUT_POLICY,
NFTNL_OBJ_CT_TIMEOUT_POLICY and NFTNL_OBJ_CT_TIMEOUT_DATA look
redundant, actually one of the is unused.
[...]
> diff --git a/src/libnftnl.map b/src/libnftnl.map
> index 0d6b20c..8e2e544 100644
> --- a/src/libnftnl.map
> +++ b/src/libnftnl.map
> @@ -340,6 +340,8 @@ LIBNFTNL_7 {
> nftnl_flowtable_list_add_tail;
> nftnl_flowtable_list_del;
> nftnl_flowtable_list_foreach;
> + nftnl_timeout_policy_attr_set_u32;
> + nftnl_obj_get_void;
I would prefer you use the existing nftnl_obj_get_data() to fetch the
array of int that represents the timeout policy. So we don't need
nftnl_obj_get_void().
> diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> new file mode 100644
> index 0000000..cd6cbc7
> --- /dev/null
> +++ b/src/obj/ct_timeout.c
> @@ -0,0 +1,371 @@
> +/*
> + * (C) 2017 Red Hat GmbH
> + * Author: Florian Westphal <[email protected]>
If this is based on Florian's work, you can refer to that.
But I think you've written this code, right?
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <arpa/inet.h>
> +#include <errno.h>
> +#include <inttypes.h>
> +
> +#include <linux/netfilter/nf_tables.h>
> +
> +#include "internal.h"
> +#include <libmnl/libmnl.h>
> +#include <libnftnl/object.h>
> +#include <libnftnl/cttimeout.h>
> +
> +#include "obj.h"
> +
> +static const char *const tcp_state_to_name[] = {
> + [NFTA_CT_TIMEOUT_ATTR_TCP_SYN_SENT] = "SYN_SENT",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_SYN_RECV] = "SYN_RECV",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_ESTABLISHED] = "ESTABLISHED",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_FIN_WAIT] = "FIN_WAIT",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_CLOSE_WAIT] = "CLOSE_WAIT",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_LAST_ACK] = "LAST_ACK",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_TIME_WAIT] = "TIME_WAIT",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_CLOSE] = "CLOSE",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_SYN_SENT2] = "SYN_SENT2",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_RETRANS] = "RETRANS",
> + [NFTA_CT_TIMEOUT_ATTR_TCP_UNACK] = "UNACKNOWLEDGED",
As said, use enum tcp_conntrack instead of these NFTA_CT_* that will
be now gone.
[...]
> +static struct {
> + uint32_t attr_max;
> + const char *const *state_to_name;
> + uint32_t *dflt_timeout;
> +} timeout_protocol[IPPROTO_MAX] = {
> + [IPPROTO_TCP] = {
> + .attr_max = NFTA_CT_TIMEOUT_ATTR_TCP_MAX,
> + .state_to_name = tcp_state_to_name,
> + .dflt_timeout = tcp_dflt_timeout,
> + },
> + [IPPROTO_UDP] = {
> + .attr_max = NFTA_CT_TIMEOUT_ATTR_UDP_MAX,
> + .state_to_name = udp_state_to_name,
> + .dflt_timeout = udp_dflt_timeout,
> + },
> +};
> +
> +
Double line break, one single it just fine.
> +struct _container_policy_cb {
> + unsigned int nlattr_max;
> + void *tb;
> +};
> +
> +int
> +nftnl_timeout_policy_attr_set_u32(struct nftnl_obj *e,
> + uint32_t type, uint32_t data)
> +{
> + struct nftnl_obj_ct_timeout *t = nftnl_obj_data(e);
> + size_t timeout_array_size;
Missing line break between variable definition and function body.
> + /* Layer 4 protocol needs to be already set. */
> + if (!(e->flags & (1 << NFTNL_OBJ_CT_TIMEOUT_L4PROTO)))
> + return -1;
> + if (t->timeout == NULL) {
> + /* if not supported, default to generic protocol tracker. */
> + if (timeout_protocol[t->l4proto].attr_max != 0) {
> + timeout_array_size = sizeof(uint32_t) *
> + timeout_protocol[t->l4proto].attr_max;
> + } else {
> + timeout_array_size = sizeof(uint32_t) *
> + timeout_protocol[IPPROTO_RAW].attr_max;
> + }
> + t->timeout = calloc(1, timeout_array_size);
> + if (t->timeout == NULL)
> + return -1;
> + }
> +
> + /* this state does not exists in this protocol tracker.*/
> + if (type > timeout_protocol[t->l4proto].attr_max)
> + return -1;
> +
> + t->timeout[type] = data;
> + t->polset |= (1 << type);
> +
> + if (!(e->flags & (1 << NFTNL_OBJ_CT_TIMEOUT_POLICY)))
> + e->flags |= (1 << NFTNL_OBJ_CT_TIMEOUT_POLICY);
> + if (!(e->flags & (1 << NFTNL_OBJ_CT_TIMEOUT_DATA)))
> + e->flags |= (1 << NFTNL_OBJ_CT_TIMEOUT_DATA);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(nftnl_timeout_policy_attr_set_u32);
Place EXPORT_SYMBOL() before the function, otherwise clang breaks. See
other spots in this library.
[...]
> +static int nftnl_obj_ct_timeout_snprintf_default(char *buf, size_t len,
> + const struct nftnl_obj *e)
> +{
> + int ret = 0;
> + int offset = 0, remain = len;
> +
> + struct nftnl_obj_ct_timeout *timeout = nftnl_obj_data(e);
> +
> + if (e->flags & (1 << NFTNL_OBJ_CT_TIMEOUT_L3PROTO)) {
> + ret = snprintf(buf+offset, len, "family %d ",
ret = snprintf(buf + offset, ...)
is preferred for new code, I mean, "buf + offset" with space between
variables.
I know other code doesn't follow this, but better if we fix coding
style in new code.
[...]
> diff --git a/src/object.c b/src/object.c
> index d8278f3..988c17d 100644
> --- a/src/object.c
> +++ b/src/object.c
[...]
> @@ -206,6 +207,12 @@ uint32_t nftnl_obj_get_u32(struct nftnl_obj *obj,
> uint16_t attr)
> return ret == NULL ? 0 : *((uint32_t *)ret);
> }
>
> +EXPORT_SYMBOL(nftnl_obj_get_void);
> +void *nftnl_obj_get_void(struct nftnl_obj *obj, uint16_t attr)
> +{
> + return (void *)nftnl_obj_get(obj, attr);
> +}
> +
> EXPORT_SYMBOL(nftnl_obj_get_u64);
> uint64_t nftnl_obj_get_u64(struct nftnl_obj *obj, uint16_t attr)
> {
> @@ -454,7 +461,8 @@ static int nftnl_obj_snprintf_dflt(char *buf, size_t size,
> obj);
> SNPRINTF_BUFFER_SIZE(ret, remain, offset);
> }
> - ret = snprintf(buf + offset, offset, "]");
> +
> + ret = snprintf(buf + strlen(buf), offset, "]");
Hm, there must be a reason for this change. Otherwise, remove it.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
