Re: Bugfix: libnetfilter_conntrack getter and setter

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

Hi,

Victor Stinner wrote:
> Libnetfilter_conntrack getters and setters of new API are not complete:
>  * it's not possible to set counter attributes value

Because of ctnetlink, the kernel part of this whole thing, doesn't
support this. Anyway, as you pointed out below, I can't see how this
could be useful.

>  * it's not possible to set or read 'use' and 'id' attributes value

The 'use' attribute must be possible to be get, but not set. I'll commit
the patch for the getter mangled, I prefer dropping the 'id' support
since it's planned to be removed.

> I can understand that setting counter values is not very useful, but trying to 
> set them would lead to a crash (call NULL function). Same problem when trying 
> to read use/id attribute value.

OK, I think that the patch attached should be enough.

> An alternative for nfct_set_attr() is to do nothing if the getter in NULL (and 
> set an error?).

I prefer doing nothing and documenting this issue, perhaps doing some
kind of warning or assertion, although that would be too much I think.
Moreover, the error thing would pollute the code with tons of error
checkings in the set operations.

Thanks again Victor.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1527 bytes --]

Index: src/conntrack/api.c
===================================================================
--- src/conntrack/api.c	(revisión: 6716)
+++ src/conntrack/api.c	(copia de trabajo)
@@ -185,6 +185,12 @@
  * @ct: pointer to a valid conntrack
  * @type: attribute type
  * @value: pointer to the attribute value
+ *
+ * Note that certain attributes are unsettable:
+ * 	- ATTR_USE
+ * 	- ATTR_ID
+ * 	- ATTR_*_COUNTER_*
+ * The call of this function for such attributes do nothing.
  */
 void nfct_set_attr(struct nf_conntrack *ct,
 		   const enum nf_conntrack_attr type, 
@@ -196,8 +202,10 @@
 	if (type >= ATTR_MAX)
 		return;
 
-	set_attr_array[type](ct, value);
-	set_bit(type, ct->set);
+	if (set_attr_array[type]) {
+		set_attr_array[type](ct, value);
+		set_bit(type, ct->set);
+	}
 }
 
 /**
Index: src/conntrack/getter.c
===================================================================
--- src/conntrack/getter.c	(revisión: 6716)
+++ src/conntrack/getter.c	(copia de trabajo)
@@ -162,6 +162,11 @@
 	return &ct->status;
 }
 
+static const void *get_attr_use(const struct nf_conntrack *ct)
+{
+	return &ct->use;
+}
+
 get_attr get_attr_array[] = {
 	[ATTR_ORIG_IPV4_SRC]		= get_attr_orig_ipv4_src,
 	[ATTR_ORIG_IPV4_DST] 		= get_attr_orig_ipv4_dst,
@@ -193,5 +198,6 @@
 	[ATTR_ORIG_COUNTER_BYTES]	= get_attr_orig_counter_bytes,
 	[ATTR_REPL_COUNTER_PACKETS]	= get_attr_repl_counter_packets,
 	[ATTR_REPL_COUNTER_BYTES]	= get_attr_repl_counter_bytes,
+	[ATTR_USE]			= get_attr_use,
 	[ATTR_STATUS]			= get_attr_status,
 };