Skip to contentSkip to conversation

libnfnetlink new API #2

RFC PATCH In review Pablo Neira Ayuso 2 participants 3 messages Jul 25, 2006 – Aug 21, 2006 netfilter

lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

RFC PATCH 1/1 libnfnetlink new API #2

Hi,

Since I'll be leaving for two weeks, I'd like to put a patch for libnfnetlink on the table that I'm currently distributing with conntrackd for further discussion. I'd like to see this patch or something similar in mainline someday.

This patch:

- Fixes error handling that is currently broken, errors are now reported via errno so everyone could use perror(...) to get a more detailed description to know what is going wrong. Basically the new functions return -1 and set errno appropiately.

- Adds Documentation that comes handy for developers.

- Introduces replacement for nfnl_listen (nfnl_receive_process) and for nfnl_talk (nfnl_send_received_process), both are integrated with the nfnl_subsys_handle logic introduced by Harald, that IMHO must be the right direction, and set errno appropiately in case of error. These new functions obsolete nfnl_listen and nfnl_talk, we can add a clause __deprecated to warn programmers without removing them.

- Iterator API: to loop over a multipart netlink message and process it. This gives more control in the message processing. It is similar to Harald's nfnl_get_first_msg, nfnl_get_msg_next and nfnl_handle_packet set of functions but sets errno and move iterator private information out of nfnl_handle. I must confess that in this case I don't like too much the idea of providing too many function to do the same but my API looks friendlier I think, programmers are familiar with the concept of iterators.

- Introduce assertions to check input data: This can catch up wrong use of the API and errors and the application can break "nicer" (if breakages would ever be nice...) that segfaulting. I have seen these in others libraries.

In short: I think that we can deprecate old functions (just adding a warning in compilation time) and remove them in version 2, I have seen this in other libraries: we maintaining an old version 1 for those that don't want to move forward some time but provide a clean version 2 and drop early design errors. BTW, probably the name of some functions are ugly, I accept suggestions ;)

@Patrick: I think that Harald has more in-deep knowledge about the libraries but, since he's really busy these days, your impressions on this issue can be also worth as well.

Signature · 1 line
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
libnfnetlink/libnfnetlink.h +30 -0
Index: include/libnfnetlink/libnfnetlink.h
===================================================================
@@ -98,6 +98,36 @@
 					  const unsigned char *buf,
 					  size_t len);
 
+/* join a certain netlink multicast group */
+extern int nfnl_join(const struct nfnl_handle *nfnlh, unsigned int group);
+
+/* process a netlink message */
+extern int nfnl_process(struct nfnl_handle *h,
+			const unsigned char *buf,
+			size_t len);
+
+/* iterator API */
+
+extern struct nfnl_iterator *
+nfnl_iterator_create(const struct nfnl_handle *h,
+		     const char *buf,
+		     size_t len);
+
+extern void nfnl_iterator_destroy(struct nfnl_iterator *it);
+
+extern int nfnl_iterator_process(struct nfnl_handle *h,
+				 struct nfnl_iterator *it);
+
+extern int nfnl_iterator_next(const struct nfnl_handle *h,
+			      struct nfnl_iterator *it);
+
+/* replacement for nfnl_listen */
+extern int nfnl_receive_process(struct nfnl_handle *h);
+
+/* replacement for nfnl_talk */
+extern int nfnl_send_receive_process(struct nfnl_handle *h,
+				     struct nlmsghdr *nlh);
+
 #define nfnl_attr_present(tb, attr)			\
 	(tb[attr-1])
libnfnetlink.c +584 -61

951 lines not on this page · raw message

#1 lore raw
Quoted 44 lines · Pablo Neira Ayuso

Hi,

Since I'll be leaving for two weeks, I'd like to put a patch for libnfnetlink on the table that I'm currently distributing with conntrackd for further discussion. I'd like to see this patch or something similar in mainline someday.

This patch:

- Fixes error handling that is currently broken, errors are now reported via errno so everyone could use perror(...) to get a more detailed description to know what is going wrong. Basically the new functions return -1 and set errno appropiately.

- Adds Documentation that comes handy for developers.

- Introduces replacement for nfnl_listen (nfnl_receive_process) and for nfnl_talk (nfnl_send_received_process), both are integrated with the nfnl_subsys_handle logic introduced by Harald, that IMHO must be the right direction, and set errno appropiately in case of error. These new functions obsolete nfnl_listen and nfnl_talk, we can add a clause __deprecated to warn programmers without removing them.

- Iterator API: to loop over a multipart netlink message and process it. This gives more control in the message processing. It is similar to Harald's nfnl_get_first_msg, nfnl_get_msg_next and nfnl_handle_packet set of functions but sets errno and move iterator private information out of nfnl_handle. I must confess that in this case I don't like too much the idea of providing too many function to do the same but my API looks friendlier I think, programmers are familiar with the concept of iterators.

- Introduce assertions to check input data: This can catch up wrong use of the API and errors and the application can break "nicer" (if breakages would ever be nice...) that segfaulting. I have seen these in others libraries.

In short: I think that we can deprecate old functions (just adding a warning in compilation time) and remove them in version 2, I have seen this in other libraries: we maintaining an old version 1 for those that don't want to move forward some time but provide a clean version 2 and drop early design errors. BTW, probably the name of some functions are ugly, I accept suggestions ;)

Quoted 3 lines

@Patrick: I think that Harald has more in-deep knowledge about the libraries but, since he's really busy these days, your impressions on this issue can be also worth as well.

Without commenting on deprecating functions (I don't know about that), the patch looks good to me.

Quoted 16 lines

+ * nfnl_send_receive_process - request/response challenge
+ * @h: nfnetlink handler
+ * @nlh: nfnetlink message to be sent
+ *
+ * This function is sends a nfnetlink message to a certain subsystem
+ * and receives the response that is processed by the callback registered
+ * via register_callback(). Note that this function is a replacement for
+ * nfnl_talk, its use is recommended.
+ *
+ * On success, 0 is returned. On error, a negative is returned. If your
+ * does not want to listen to events anymore, then it must return a value
+ * lesser or equal to 0.
+ *
+ * Note that ENOBUFS is returned in case that nfnetlink is exhausted. In
+ * that case is possible that the information requested is incomplete.
+ */

libnfnetlink.c line 1521

Quoted 10 lines
libnfnetlink.c +10 -0
+int nfnl_send_receive_process(struct nfnl_handle *h, struct nlmsghdr *nlh)
+{
+	assert(h);
+	assert(nlh);
+
+	if (nfnl_send(h, nlh) == -1)
+		return -1;
+
+	return nfnl_receive_process(h);
+}

This doesn't really do what it promises, it will call the callback for any message it receives, not only a response. We need to start using sequence numbers before we associate responses with queries.

author maintainer
#2 lore raw
Quoted 46 lines · Patrick McHardy
Quoted 44 lines · Pablo Neira Ayuso

Hi,

Since I'll be leaving for two weeks, I'd like to put a patch for libnfnetlink on the table that I'm currently distributing with conntrackd for further discussion. I'd like to see this patch or something similar in mainline someday.

This patch:

- Fixes error handling that is currently broken, errors are now reported via errno so everyone could use perror(...) to get a more detailed description to know what is going wrong. Basically the new functions return -1 and set errno appropiately.

- Adds Documentation that comes handy for developers.

- Introduces replacement for nfnl_listen (nfnl_receive_process) and for nfnl_talk (nfnl_send_received_process), both are integrated with the nfnl_subsys_handle logic introduced by Harald, that IMHO must be the right direction, and set errno appropiately in case of error. These new functions obsolete nfnl_listen and nfnl_talk, we can add a clause __deprecated to warn programmers without removing them.

- Iterator API: to loop over a multipart netlink message and process it. This gives more control in the message processing. It is similar to Harald's nfnl_get_first_msg, nfnl_get_msg_next and nfnl_handle_packet set of functions but sets errno and move iterator private information out of nfnl_handle. I must confess that in this case I don't like too much the idea of providing too many function to do the same but my API looks friendlier I think, programmers are familiar with the concept of iterators.

- Introduce assertions to check input data: This can catch up wrong use of the API and errors and the application can break "nicer" (if breakages would ever be nice...) that segfaulting. I have seen these in others libraries.

In short: I think that we can deprecate old functions (just adding a warning in compilation time) and remove them in version 2, I have seen this in other libraries: we maintaining an old version 1 for those that don't want to move forward some time but provide a clean version 2 and drop early design errors. BTW, probably the name of some functions are ugly, I accept suggestions ;)

Quoted 8 lines
Quoted 3 lines

@Patrick: I think that Harald has more in-deep knowledge about the libraries but, since he's really busy these days, your impressions on this issue can be also worth as well.

Without commenting on deprecating functions (I don't know about that), the patch looks good to me.

My idea is to introduce something like this to deprecate functions:

+/* Deprecated API, keep it to ensure backward compatibility */ +
+#if __GNUC_MINOR__ > 0
+# ifndef __deprecated
+# define __deprecated __attribute__((deprecated))
+# endif
+#endif
+
+extern int __deprecated nfnl_handle_packet(struct nfnl_handle *, char *buf,
+ int len);
+

Quoted 23 lines
Quoted 23 lines

+ * nfnl_send_receive_process - request/response challenge
+ * @h: nfnetlink handler
+ * @nlh: nfnetlink message to be sent
+ *
+ * This function is sends a nfnetlink message to a certain subsystem
+ * and receives the response that is processed by the callback registered
+ * via register_callback(). Note that this function is a replacement for
+ * nfnl_talk, its use is recommended.
+ *
+ * On success, 0 is returned. On error, a negative is returned. If your
+ * does not want to listen to events anymore, then it must return a value
+ * lesser or equal to 0.
+ *
+ * Note that ENOBUFS is returned in case that nfnetlink is exhausted. In
+ * that case is possible that the information requested is incomplete.
+ */

libnfnetlink.c +7 -0
+int nfnl_send_receive_process(struct nfnl_handle *h, struct nlmsghdr *nlh)
+{
+	assert(h);
+	assert(nlh);
+
+	if (nfnl_send(h, nlh) == -1)
+		return -1;
Quoted 8 lines

libnfnetlink.c line 1521

Quoted 3 lines
libnfnetlink.c +3 -0
+
+	return nfnl_receive_process(h);
+}

This doesn't really do what it promises, it will call the callback for any message it receives, not only a response. We need to start using sequence numbers before we associate responses with queries.

Hm, then it is my English that is broken, I supposed that response means every message received from the subsystem :(

Signature · 1 line
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