Patchwork [5/5] nfacct: add timestamp option

login
register
mail settings
Submitter Eric Leblond
Date Aug. 1, 2012, 9:27 p.m.
Message ID <1343856436-11129-6-git-send-email-eric@regit.org>
Download mbox | patch
Permalink /patch/174607/
State Accepted
Headers show

Comments

Eric Leblond - Aug. 1, 2012, 9:27 p.m.
This patch adds a timestamp option to the nfacct plugin.
If activated, nfacct output a timestamp which is computed just
after sending the nfacct request.
---
 input/sum/ulogd_inpflow_NFACCT.c |   35 ++++++++++++++++++++++++++++++++++-
 ulogd.conf.in                    |    2 ++
 2 files changed, 36 insertions(+), 1 deletion(-)
Pablo Neira - Aug. 3, 2012, 9:35 a.m.
On Wed, Aug 01, 2012 at 11:27:16PM +0200, Eric Leblond wrote:
> This patch adds a timestamp option to the nfacct plugin.
> If activated, nfacct output a timestamp which is computed just
> after sending the nfacct request.

I think it makes sense to make it unconditionally.

The dump of the counters without the time doesn't make too much sense
to me?

Let me know.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Leblond - Aug. 3, 2012, 9:43 a.m.
Hello,

Le vendredi 03 août 2012 à 11:35 +0200, Pablo Neira Ayuso a écrit :
> On Wed, Aug 01, 2012 at 11:27:16PM +0200, Eric Leblond wrote:
> > This patch adds a timestamp option to the nfacct plugin.
> > If activated, nfacct output a timestamp which is computed just
> > after sending the nfacct request.
> 
> I think it makes sense to make it unconditionally.
> 
> The dump of the counters without the time doesn't make too much sense
> to me?
> 
> Let me know.

I think it was not originally added because some output format did not
need it. Maybe we can make output of a timestamp by default and let
people who want to spare the cycles of gettimeofday() set it to 0.

BR,
Pablo Neira - Aug. 3, 2012, 11:24 a.m.
On Fri, Aug 03, 2012 at 11:43:58AM +0200, Eric Leblond wrote:
> Hello,
> 
> Le vendredi 03 août 2012 à 11:35 +0200, Pablo Neira Ayuso a écrit :
> > On Wed, Aug 01, 2012 at 11:27:16PM +0200, Eric Leblond wrote:
> > > This patch adds a timestamp option to the nfacct plugin.
> > > If activated, nfacct output a timestamp which is computed just
> > > after sending the nfacct request.
> > 
> > I think it makes sense to make it unconditionally.
> > 
> > The dump of the counters without the time doesn't make too much sense
> > to me?
> > 
> > Let me know.
> 
> I think it was not originally added because some output format did not
> need it.

I guess you refer to GPRINT.

> Maybe we can make output of a timestamp by default and let
> people who want to spare the cycles of gettimeofday() set it to 0.

Hm, that means that we'll have two gettimeofday in case that GPRINT is
used (specifically if people configure this wrong, ie. setting on
timestamp both in GPRINT and NFACCT, that's likely to happen IMO).

I'm still undecided, but it seems to me we provide more fine-grain
control if timestamp is calculated in the output for the nfacct case.

I think we have to agree where to calculate the timestamp (input or
output plugins?) for consistency, or at least agreed on some policy
for this.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - Aug. 3, 2012, 2:54 p.m.
On Fri, Aug 03, 2012 at 01:24:49PM +0200, Pablo Neira Ayuso wrote:
[...]
> I'm still undecided, but it seems to me we provide more fine-grain
> control if timestamp is calculated in the output for the nfacct case.

Well, most input plugins already provide timestamp calculation. GPRINT
seems to be the exception for the output plugin case.

I have applied the patch expanding the documentation on the example
file a bit.

Thanks Eric.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mr Dash Four - Sept. 1, 2012, 12:49 p.m.
Pablo Neira Ayuso wrote:
> On Wed, Aug 01, 2012 at 11:27:16PM +0200, Eric Leblond wrote:
>> This patch adds a timestamp option to the nfacct plugin.
>> If activated, nfacct output a timestamp which is computed just
>> after sending the nfacct request.
> 
> I think it makes sense to make it unconditionally.
Yep. The PGSQL output plugin can't function without it. When I have the following:

stack=acct1:NFACCT,pgsql3:PGSQL

I get the following error if timestamp is not enabled:

<7> ulogd.c:700 type mismatch between PGSQL and NFACCT in stack
<7> ulogd.c:727 cannot find key `oob.time.sec' in stack

I had to dig in further to find out why that was the case (something I could have done without), so I think it makes sense for the timestamp to always be included.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mr Dash Four - Sept. 2, 2012, 8:03 p.m.
> stack=acct1:NFACCT,pgsql3:PGSQL
>
> I get the following error if timestamp is not enabled:
>
> <7> ulogd.c:700 type mismatch between PGSQL and NFACCT in stack
> <7> ulogd.c:727 cannot find key `oob.time.sec' in stack
>
> I had to dig in further to find out why that was the case (something I could have done without), so I think it makes sense for the timestamp to always be included.
>   
I am still getting a mismatch error, though there is no indication as to 
what is causing it (full debug log follows):


<1> ulogd.c:820 building new pluginstance stack (acct1:NFACCT,pgsql3:PGSQL):
<1> ulogd.c:829 tok=`acct1:NFACCT'
<1> ulogd.c:866 pushing `NFACCT' on stack
<1> ulogd.c:829 tok=`pgsql3:PGSQL'
<1> ulogd.c:866 pushing `PGSQL' on stack
<1> ulogd.c:646 traversing plugin `PGSQL'
<5> ../../util/db.c:146 (re)configuring
<1> ulogd_output_PGSQL.c:146 SELECT attname FROM nfacct
<1> ulogd_output_PGSQL.c:166 5 fields in table
<1> ulogd.c:646 traversing plugin `NFACCT'
<1> ulogd.c:662 connecting input/output keys of stack:
<1> ulogd.c:670 traversing plugin `PGSQL'
<7> ulogd.c:700 type mismatch between PGSQL and NFACCT in stack
<1> ulogd.c:627 acct1(NFACCT)
<1> ulogd.c:733 assigning `sum.name(?)' as source for PGSQL(sum.name)
<1> ulogd.c:627 acct1(NFACCT)
<1> ulogd.c:733 assigning `sum.pkts(?)' as source for PGSQL(sum.pkts)
<1> ulogd.c:627 acct1(NFACCT)
<1> ulogd.c:733 assigning `sum.bytes(?)' as source for PGSQL(sum.bytes)
<1> ulogd.c:627 acct1(NFACCT)
<1> ulogd.c:733 assigning `oob.time.sec(?)' as source for 
PGSQL(oob.time.sec)
<1> ulogd.c:627 acct1(NFACCT)
<1> ulogd.c:733 assigning `oob.time.usec(?)' as source for 
PGSQL(oob.time.usec)
<1> ulogd.c:670 traversing plugin `NFACCT'
<5> ../../util/db.c:180 starting
<1> ../../util/db.c:86 allocating 593 bytes for statement
<1> ../../util/db.c:133 stmt='SELECT INSERT_NFACCT('

Any ideas?

Also, not nfacct related, but a general question about ulogd2: What 
happens when I have my iptables NFLOG targets set/created, but the ulog 
daemon is not running - do the packets matching get discarded or do they 
get buffered (if so, how large is this buffer and is this configurable?) 
so that when ulogd2 gets started they get processed?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/input/sum/ulogd_inpflow_NFACCT.c b/input/sum/ulogd_inpflow_NFACCT.c
index a3bcc72..66e9f78 100644
--- a/input/sum/ulogd_inpflow_NFACCT.c
+++ b/input/sum/ulogd_inpflow_NFACCT.c
@@ -30,6 +30,7 @@  struct nfacct_pluginstance {
 	uint32_t		seq;
 	struct ulogd_fd		ufd;
 	struct ulogd_timer	timer;
+	struct timeval tv;
 };
 
 static struct config_keyset nfacct_kset = {
@@ -45,18 +46,27 @@  static struct config_keyset nfacct_kset = {
 			.type	 = CONFIG_TYPE_INT,
 			.options = CONFIG_OPT_NONE,
 			.u.value = 1,
+		},
+		{
+			.key	 = "timestamp",
+			.type	 = CONFIG_TYPE_INT,
+			.options = CONFIG_OPT_NONE,
+			.u.value = 0,
 		}
 	},
-	.num_ces = 2,
+	.num_ces = 3,
 };
 #define pollint_ce(x)	(x->ces[0])
 #define zerocounter_ce(x) (x->ces[1])
+#define timestamp_ce(x) (x->ces[2])
 
 enum ulogd_nfacct_keys {
 	ULOGD_NFACCT_NAME,
 	ULOGD_NFACCT_PKTS,
 	ULOGD_NFACCT_BYTES,
 	ULOGD_NFACCT_RAW,
+	ULOGD_NFACCT_TIME_SEC,
+	ULOGD_NFACCT_TIME_USEC,
 };
 
 static struct ulogd_key nfacct_okeys[] = {
@@ -80,12 +90,27 @@  static struct ulogd_key nfacct_okeys[] = {
 		.flags	= ULOGD_RETF_NONE,
 		.name	= "sum",
 	},
+	[ULOGD_NFACCT_TIME_SEC] = {
+		.type = ULOGD_RET_UINT32,
+		.flags = ULOGD_RETF_NONE,
+		.name = "oob.time.sec",
+		.ipfix = {
+			.vendor = IPFIX_VENDOR_IETF,
+			.field_id = 22
+		},
+	},
+	[ULOGD_NFACCT_TIME_USEC] = {
+		.type = ULOGD_RET_UINT32,
+		.flags = ULOGD_RETF_NONE,
+		.name = "oob.time.usec",
+	},
 };
 
 static void
 propagate_nfacct(struct ulogd_pluginstance *upi, struct nfacct *nfacct)
 {
 	struct ulogd_key *ret = upi->output.keys;
+	struct nfacct_pluginstance *cpi = (struct nfacct_pluginstance *) upi->private;
 
 	okey_set_ptr(&ret[ULOGD_NFACCT_NAME],
 			(void *)nfacct_attr_get_str(nfacct, NFACCT_ATTR_NAME));
@@ -94,6 +119,10 @@  propagate_nfacct(struct ulogd_pluginstance *upi, struct nfacct *nfacct)
 	okey_set_u64(&ret[ULOGD_NFACCT_BYTES],
 			nfacct_attr_get_u64(nfacct, NFACCT_ATTR_BYTES));
 	okey_set_ptr(&ret[ULOGD_NFACCT_RAW], nfacct);
+	if (timestamp_ce(upi->config_kset).u.value != 0) {
+		okey_set_u32(&ret[ULOGD_NFACCT_TIME_SEC], cpi->tv.tv_sec);
+		okey_set_u32(&ret[ULOGD_NFACCT_TIME_USEC], cpi->tv.tv_usec);
+	}
 
 	ulogd_propagate_results(upi);
 }
@@ -174,6 +203,10 @@  static int nfacct_send_request(struct ulogd_pluginstance *upi)
 		ulogd_log(ULOGD_ERROR, "Cannot send netlink message\n");
 		return -1;
 	}
+	if (timestamp_ce(upi->config_kset).u.value != 0) {
+		/* Compute time of query */
+		gettimeofday(&cpi->tv, NULL);
+	}
 	return 0;
 }
 
diff --git a/ulogd.conf.in b/ulogd.conf.in
index 0e45714..e99212f 100644
--- a/ulogd.conf.in
+++ b/ulogd.conf.in
@@ -273,3 +273,5 @@  mark = 1
 pollinterval = 2
 # Set to zero to avoid zeroing counters after read
 #zerocounter = 0
+# Output timestamp if set to 1 (default 0)
+#timestamp = 1