[PATCH 0/2] iproute2: add GENL helpers

[PATCH 0/2] iproute2: add GENL helpers

[Julian Anastasov]

	Put common code for GENL users in new files.
I'll resend the tcp_metrics support for iproute2 when
the kernel part is in mainline.

Julian Anastasov (2):
  iproute2: add libgenl files
  iproute2: use libgenl in ipl2tp

 include/libgenl.h |   25 ++++++++++++++++++++
 ip/ipl2tp.c       |   65 ++--------------------------------------------------
 lib/Makefile      |    2 +-
 lib/libgenl.c     |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 63 deletions(-)
 create mode 100644 include/libgenl.h
 create mode 100644 lib/libgenl.c

-- 
1.7.3.4

[PATCH 1/2] iproute2: add libgenl files

[Julian Anastasov]

	Create libgenl.h and libgenl.c. They will contain
common code for GENL users such as ipl2tp, tcp_metrics, etc.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
 include/libgenl.h |   25 ++++++++++++++++++++
 lib/Makefile      |    2 +-
 lib/libgenl.c     |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 1 deletions(-)
 create mode 100644 include/libgenl.h
 create mode 100644 lib/libgenl.c

diff --git a/include/libgenl.h b/include/libgenl.h
new file mode 100644
index 0000000..ef455c1
--- /dev/null
+++ b/include/libgenl.h
@@ -0,0 +1,25 @@
+#ifndef __LIBGENL_H__
+#define __LIBGENL_H__
+
+#include "libnetlink.h"
+
+#define GENL_DEFINE_REQUEST(req, hdrsize, bufsiz)			\
+struct {								\
+	struct nlmsghdr		n;					\
+	struct genlmsghdr	g;					\
+	char			buf[NLMSG_ALIGN(hdrsize) + bufsiz];	\
+} req
+
+#define GENL_INIT_REQUEST(req, family, hdrsize, ver, cmd_, flags)	\
+	do {								\
+		memset(&req, 0, sizeof(req));				\
+		req.n.nlmsg_type = family;				\
+		req.n.nlmsg_flags = flags;				\
+		req.n.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN + hdrsize);	\
+		req.g.cmd = cmd_;					\
+		req.g.version = ver;					\
+	} while (0)
+
+int libgenl_resolve_family(struct rtnl_handle *grth, const char *family);
+
+#endif /* __LIBGENL_H__ */
diff --git a/lib/Makefile b/lib/Makefile
index da2f0fc..bfbe672 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -2,7 +2,7 @@ CFLAGS += -fPIC
 
 UTILOBJ=utils.o rt_names.o ll_types.o ll_proto.o ll_addr.o inet_proto.o
 
-NLOBJ=ll_map.o libnetlink.o
+NLOBJ=libgenl.o ll_map.o libnetlink.o
 
 all: libnetlink.a libutil.a
 
diff --git a/lib/libgenl.c b/lib/libgenl.c
new file mode 100644
index 0000000..c1bf399
--- /dev/null
+++ b/lib/libgenl.c
@@ -0,0 +1,65 @@
+/*
+ * libgenl.c	GENL library
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <linux/genetlink.h>
+#include "libgenl.h"
+
+static int libgenl_parse_getfamily(struct nlmsghdr *nlh)
+{
+	struct rtattr *tb[CTRL_ATTR_MAX + 1];
+	struct genlmsghdr *ghdr = NLMSG_DATA(nlh);
+	int len = nlh->nlmsg_len;
+	struct rtattr *attrs;
+
+	if (nlh->nlmsg_type != GENL_ID_CTRL) {
+		fprintf(stderr, "Not a controller message, nlmsg_len=%d "
+			"nlmsg_type=0x%x\n", nlh->nlmsg_len, nlh->nlmsg_type);
+		return -1;
+	}
+
+	len -= NLMSG_LENGTH(GENL_HDRLEN);
+
+	if (len < 0) {
+		fprintf(stderr, "wrong controller message len %d\n", len);
+		return -1;
+	}
+
+	if (ghdr->cmd != CTRL_CMD_NEWFAMILY) {
+		fprintf(stderr, "Unknown controller command %d\n", ghdr->cmd);
+		return -1;
+	}
+
+	attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN);
+	parse_rtattr(tb, CTRL_ATTR_MAX, attrs, len);
+
+	if (tb[CTRL_ATTR_FAMILY_ID] == NULL) {
+		fprintf(stderr, "Missing family id TLV\n");
+		return -1;
+	}
+
+	return rta_getattr_u16(tb[CTRL_ATTR_FAMILY_ID]);
+}
+
+int libgenl_resolve_family(struct rtnl_handle *grth, const char *family)
+{
+	GENL_DEFINE_REQUEST(req, 0, 1024);
+
+	GENL_INIT_REQUEST(req, GENL_ID_CTRL, 0, 0, CTRL_CMD_GETFAMILY,
+			  NLM_F_REQUEST);
+
+	addattr_l(&req.n, 1024, CTRL_ATTR_FAMILY_NAME,
+		  family, strlen(family) + 1);
+
+	if (rtnl_talk(grth, &req.n, 0, 0, &req.n) < 0) {
+		fprintf(stderr, "Error talking to the kernel\n");
+		return -2;
+	}
+
+	return libgenl_parse_getfamily(&req.n);
+}
+
-- 
1.7.3.4

Re: [PATCH 1/2] iproute2: add libgenl files

[Stephen Hemminger]

On Sat,  8 Sep 2012 12:48:24 +0300
Julian Anastasov <ja@ssi.bg> wrote:

> +
> +#define GENL_INIT_REQUEST(req, family, hdrsize, ver, cmd_, flags)	\
> +	do {								\
> +		memset(&req, 0, sizeof(req));				\
> +		req.n.nlmsg_type = family;				\
> +		req.n.nlmsg_flags = flags;				\
> +		req.n.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN + hdrsize);	\
> +		req.g.cmd = cmd_;					\
> +		req.g.version = ver;					\
> +	} while (0)
> +

Why not an inline function, macro code is error prone.

Re: [PATCH 1/2] iproute2: add libgenl files

[Julian Anastasov]

	Hello,

On Mon, 10 Sep 2012, Stephen Hemminger wrote:

> On Sat,  8 Sep 2012 12:48:24 +0300
> Julian Anastasov <ja@ssi.bg> wrote:
> 
> > +
> > +#define GENL_INIT_REQUEST(req, family, hdrsize, ver, cmd_, flags)	\
> > +	do {								\
> > +		memset(&req, 0, sizeof(req));				\
> > +		req.n.nlmsg_type = family;				\
> > +		req.n.nlmsg_flags = flags;				\
> > +		req.n.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN + hdrsize);	\
> > +		req.g.cmd = cmd_;					\
> > +		req.g.version = ver;					\
> > +	} while (0)
> > +
> 
> Why not an inline function, macro code is error prone.

	The problem is that we define every request with
its own structure in GENL_DEFINE_REQUEST. We can use init
func instead of macro only if we define some base structure,
for example:

struct genl_header_base {
	struct nlmsghdr		n;
	struct genlmsghdr	g;
};

	But then we will have new prefix when accessing
request fields (b.): b.n... and b.g... It will go to all places
that use addattr_*, eg. addattr_l(&req.b.n, sizeof(req), ...)
and such places can be many if there are many attributes.

#define GENL_DEFINE_REQUEST(req, hdrsize, bufsiz)			\
struct {								\
	struct genl_header_base	b;					\
	char			buf[NLMSG_ALIGN(hdrsize) + bufsiz];	\
} req

int genl_init_request(struct genl_header_base *b,
	b_size, family, hdrsize, ver, cmd_, flags)

	Do you prefer this new variant? There will be many
places like req.n that will be changed like req.b.n.

	I'll rename libgenl_ to genl_ when we decide what
to do with the request structure.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH 1/2] iproute2: add libgenl files

[Stephen Hemminger]

On Tue, 11 Sep 2012 00:54:00 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> > Why not an inline function, macro code is error prone.  
> 
> 	The problem is that we define every request with
> its own structure in GENL_DEFINE_REQUEST. We can use init
> func instead of macro only if we define some base structure,
> for example:
> 
> struct genl_header_base {
> 	struct nlmsghdr		n;
> 	struct genlmsghdr	g;
> };

Ok, that makes sense. I prefer C99 style initializers
in general. Maybe:

#define GENL_INIT_REQUEST(family, hdrsize, ver, cmd_, flags) {	\
	.req = { \
          .n = { \
             .nlmsg_type = (family), \
             .nlmsg_flags = (flags), \
             .nlmsg_len   = NLMSG_LENGTH(GENL_HDRLEN + (hdrsize)), \
           } \
          .g = { \
              .cmd = (cmd), \
              .version = (ver), \
           } \
          }
		

Or merge GENL_DEFINE_REQUEST and GENL_INIT_REQUEST.

These are all nits, and not that important, just want to make it as simple
as possible. So if you like it okay as it was, we can just use that.

Re: [PATCH 1/2] iproute2: add libgenl files

[Stephen Hemminger]

On Sat,  8 Sep 2012 12:48:24 +0300
Julian Anastasov <ja@ssi.bg> wrote:

> +static int libgenl_parse_getfamily(struct nlmsghdr *nlh)

I prefer just using 'genl' rather than 'libgenl' here. Shorter variable
names are clearer.

[PATCH 2/2] iproute2: use libgenl in ipl2tp

[Julian Anastasov]

	Use the common code from libgenl.c to parse family.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
 ip/ipl2tp.c |   65 ++--------------------------------------------------------
 1 files changed, 3 insertions(+), 62 deletions(-)

diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
index 1cbed8d..c69091d 100644
--- a/ip/ipl2tp.c
+++ b/ip/ipl2tp.c
@@ -25,6 +25,7 @@
 
 #include <linux/genetlink.h>
 #include <linux/l2tp.h>
+#include "libgenl.h"
 
 #include "utils.h"
 #include "ip_common.h"
@@ -747,67 +748,6 @@ static int do_show(int argc, char **argv)
 	return 0;
 }
 
-static int genl_parse_getfamily(struct nlmsghdr *nlh)
-{
-	struct rtattr *tb[CTRL_ATTR_MAX + 1];
-	struct genlmsghdr *ghdr = NLMSG_DATA(nlh);
-	int len = nlh->nlmsg_len;
-	struct rtattr *attrs;
-
-	if (nlh->nlmsg_type != GENL_ID_CTRL) {
-		fprintf(stderr, "Not a controller message, nlmsg_len=%d "
-			"nlmsg_type=0x%x\n", nlh->nlmsg_len, nlh->nlmsg_type);
-		return -1;
-	}
-
-	if (ghdr->cmd != CTRL_CMD_NEWFAMILY) {
-		fprintf(stderr, "Unknown controller command %d\n", ghdr->cmd);
-		return -1;
-	}
-
-	len -= NLMSG_LENGTH(GENL_HDRLEN);
-
-	if (len < 0) {
-		fprintf(stderr, "wrong controller message len %d\n", len);
-		return -1;
-	}
-
-	attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN);
-	parse_rtattr(tb, CTRL_ATTR_MAX, attrs, len);
-
-	if (tb[CTRL_ATTR_FAMILY_ID] == NULL) {
-		fprintf(stderr, "Missing family id TLV\n");
-		return -1;
-	}
-
-	return rta_getattr_u16(tb[CTRL_ATTR_FAMILY_ID]);
-}
-
-int genl_ctrl_resolve_family(const char *family)
-{
-	struct {
-		struct nlmsghdr         n;
-		struct genlmsghdr	g;
-		char                    buf[1024];
-	} req;
-
-	memset(&req, 0, sizeof(req));
-	req.n.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN);
-	req.n.nlmsg_flags = NLM_F_REQUEST;
-	req.n.nlmsg_type = GENL_ID_CTRL;
-	req.g.cmd = CTRL_CMD_GETFAMILY;
-
-	addattr_l(&req.n, 1024, CTRL_ATTR_FAMILY_NAME,
-		  family, strlen(family) + 1);
-
-	if (rtnl_talk(&genl_rth, &req.n, 0, 0, &req.n) < 0) {
-		fprintf(stderr, "Error talking to the kernel\n");
-		return -2;
-	}
-
-	return genl_parse_getfamily(&req.n);
-}
-
 int do_ipl2tp(int argc, char **argv)
 {
 	if (genl_family < 0) {
@@ -816,7 +756,8 @@ int do_ipl2tp(int argc, char **argv)
 			exit(1);
 		}
 
-		genl_family = genl_ctrl_resolve_family(L2TP_GENL_NAME);
+		genl_family = libgenl_resolve_family(&genl_rth,
+						     L2TP_GENL_NAME);
 		if (genl_family < 0)
 			exit(1);
 	}
-- 
1.7.3.4