All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH 0/4] RTNetlink and network device management library
@ 2021-04-26 11:19 Martin Doucha
  2021-04-26 11:19 ` [LTP] [PATCH 1/4] Add SAFE_REALLOC() helper function to LTP library Martin Doucha
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Martin Doucha @ 2021-04-26 11:19 UTC (permalink / raw)
  To: ltp

This patchset is still a work in progress and I've sent it mainly to open
discussion about network management API design. SAFE_REALLOC() and SAFE_RECV()
patches are trivial and can be merge right away, though.

The network management API has two separate parts:
- rtnetlink library (patch 3)
- device management library (patch 4)

The rtnetlink API is stateful. First you need to create a netlink context
(netlink socket with dynamic message buffer), then you can add arbitrary list
of messages and attributes and send them all at once. If you don't need to
process any complex response, you can use the shorthand send function which
automatically receives and validates ACKs for each sent message. You can reuse
the same context for multiple send calls.

Device management API is stateless, only one function call per operation.

Example how to use rtnetlink API (add new IPv4 address to eth0):
================================================================

struct nlmsghdr header = {
	.nlmsg_type = RTM_NEWADDR,
	.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL | NLM_F_ACK
};
struct ifaddrmsg info = {
	.ifa_family = AF_INET,
	.ifa_prefix = 24,
	.ifa_index = eth0_index
};
in_addr_t addr = inet_addr("192.168.0.1");
ctx = RTNL_CREATE_CONTEXT();
RTNL_ADD_MESSAGE(ctx, &header, &info, sizeof(info));
RTNL_ADD_ATTR(ctx, IFA_LOCAL, &addr, sizeof(addr));
if (!RTNL_SEND_VERIFY(ctx))
	// error handling
RTNL_FREE_CONTEXT(ctx);


Example how to create and configure veth pair using device management API:
==========================================================================

CREATE_VETH_PAIR("veth0", "veth1");
NETDEVICE_ADD_ADDRESS_INET("veth0", inet_addr("192.168.0.1"));
NETDEVICE_ADD_ADDRESS_INET("veth1", inet_addr("192.168.0.2"));
NETDEVICE_ACTIVATE("veth0", 1);
NETDEVICE_ACTIVATE("veth1", 1);
NETDEVICE_ADD_ROUTE_INET("veth0", 0, 0, inet_addr("192.168.1.0"), 24, 0);


Martin Doucha (4):
  Add SAFE_REALLOC() helper function to LTP library
  Add SAFE_RECV() helper function to LTP library
  RFC: Add rtnetlink helper library
  RFC: Add helper functions for managing network interfaces

 include/safe_net_fn.h     |   3 +
 include/tst_netdevice.h   | 120 +++++++++
 include/tst_rtnetlink.h   | 105 ++++++++
 include/tst_safe_macros.h |   5 +
 include/tst_safe_net.h    |   3 +
 lib/safe_net.c            |  25 ++
 lib/tst_netdevice.c       | 506 ++++++++++++++++++++++++++++++++++++++
 lib/tst_rtnetlink.c       | 399 ++++++++++++++++++++++++++++++
 lib/tst_safe_macros.c     |  15 ++
 9 files changed, 1181 insertions(+)
 create mode 100644 include/tst_netdevice.h
 create mode 100644 include/tst_rtnetlink.h
 create mode 100644 lib/tst_netdevice.c
 create mode 100644 lib/tst_rtnetlink.c

-- 
2.31.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 1/4] Add SAFE_REALLOC() helper function to LTP library
  2021-04-26 11:19 [LTP] [PATCH 0/4] RTNetlink and network device management library Martin Doucha
@ 2021-04-26 11:19 ` Martin Doucha
  2021-04-27 13:04   ` Cyril Hrubis
  2021-04-26 11:19 ` [LTP] [PATCH 2/4] Add SAFE_RECV() " Martin Doucha
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Martin Doucha @ 2021-04-26 11:19 UTC (permalink / raw)
  To: ltp

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 include/tst_safe_macros.h |  5 +++++
 lib/tst_safe_macros.c     | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index b9d9baa1a..d6f32ef4c 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -67,6 +67,11 @@ int safe_dup2(const char *file, const int lineno, int oldfd, int newfd);
 #define SAFE_MALLOC(size) \
 	safe_malloc(__FILE__, __LINE__, NULL, (size))
 
+void *safe_realloc(const char *file, const int lineno, void *ptr, size_t size);
+
+#define SAFE_REALLOC(ptr, size) \
+	safe_realloc(__FILE__, __LINE__, (ptr), (size))
+
 #define SAFE_MKDIR(pathname, mode) \
 	safe_mkdir(__FILE__, __LINE__, NULL, (pathname), (mode))
 
diff --git a/lib/tst_safe_macros.c b/lib/tst_safe_macros.c
index 182b690bb..fd5f1704b 100644
--- a/lib/tst_safe_macros.c
+++ b/lib/tst_safe_macros.c
@@ -5,6 +5,7 @@
 
 #define _GNU_SOURCE
 #include <unistd.h>
+#include <stdlib.h>
 #include <errno.h>
 #include <sched.h>
 #include <sys/ptrace.h>
@@ -433,6 +434,20 @@ int safe_dup2(const char *file, const int lineno, int oldfd, int newfd)
 	return rval;
 }
 
+void *safe_realloc(const char *file, const int lineno, void *ptr, size_t size)
+{
+	void *ret;
+
+	ret = realloc(ptr, size);
+
+	if (!ret) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			"realloc(%p, %zu) failed", ptr, size);
+	}
+
+	return ret;
+}
+
 sighandler_t safe_signal(const char *file, const int lineno,
 	int signum, sighandler_t handler)
 {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 2/4] Add SAFE_RECV() helper function to LTP library
  2021-04-26 11:19 [LTP] [PATCH 0/4] RTNetlink and network device management library Martin Doucha
  2021-04-26 11:19 ` [LTP] [PATCH 1/4] Add SAFE_REALLOC() helper function to LTP library Martin Doucha
@ 2021-04-26 11:19 ` Martin Doucha
  2021-04-27 13:06   ` Cyril Hrubis
  2021-04-26 11:19 ` [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library Martin Doucha
  2021-04-26 11:19 ` [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces Martin Doucha
  3 siblings, 1 reply; 14+ messages in thread
From: Martin Doucha @ 2021-04-26 11:19 UTC (permalink / raw)
  To: ltp

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 include/safe_net_fn.h  |  3 +++
 include/tst_safe_net.h |  3 +++
 lib/safe_net.c         | 25 +++++++++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/include/safe_net_fn.h b/include/safe_net_fn.h
index 2fda11fab..ff81b1337 100644
--- a/include/safe_net_fn.h
+++ b/include/safe_net_fn.h
@@ -47,6 +47,9 @@ ssize_t safe_sendto(const char *file, const int lineno, char len_strict,
 ssize_t safe_sendmsg(const char *file, const int lineno, size_t msg_len,
 		  int sockfd, const struct msghdr *msg, int flags);
 
+ssize_t safe_recv(const char *file, const int lineno, size_t len,
+	int sockfd, void *buf, size_t size, int flags);
+
 ssize_t safe_recvmsg(const char *file, const int lineno, size_t msg_len,
 		  int sockfd, struct msghdr *msg, int flags);
 
diff --git a/include/tst_safe_net.h b/include/tst_safe_net.h
index 78a488a18..e85b79a3e 100644
--- a/include/tst_safe_net.h
+++ b/include/tst_safe_net.h
@@ -42,6 +42,9 @@
 #define SAFE_SENDMSG(msg_len, fd, msg, flags) \
 	safe_sendmsg(__FILE__, __LINE__, msg_len, fd, msg, flags)
 
+#define SAFE_RECV(msg_len, fd, buf, size, flags)		\
+	safe_recv(__FILE__, __LINE__, (msg_len), (fd), (buf), (size), (flags))
+
 #define SAFE_RECVMSG(msg_len, fd, msg, flags)		\
 	safe_recvmsg(__FILE__, __LINE__, msg_len, fd, msg, flags)
 
diff --git a/lib/safe_net.c b/lib/safe_net.c
index f9ebea610..211fd9b67 100644
--- a/lib/safe_net.c
+++ b/lib/safe_net.c
@@ -273,6 +273,31 @@ ssize_t safe_sendmsg(const char *file, const int lineno, size_t len,
 	return rval;
 }
 
+ssize_t safe_recv(const char *file, const int lineno, size_t len,
+	int sockfd, void *buf, size_t size, int flags)
+{
+	ssize_t rval;
+
+	rval = recv(sockfd, buf, size, flags);
+
+	if (rval == -1) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"recv(%d, %p, %zu, %d) failed", sockfd, buf, size,
+			flags);
+	} else if (rval < 0) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"Invalid recv(%d, %p, %zu, %d) return value %zd",
+			sockfd, buf, size, flags, rval);
+	} else if (len && (size_t)rval != len) {
+		tst_brkm_(file, lineno, TBROK, NULL,
+			"recv(%d, %p, %zu, %d) ret(%zd) != len(%zu)",
+			sockfd, buf, size, flags, rval, len);
+	}
+
+	return rval;
+
+}
+
 ssize_t safe_recvmsg(const char *file, const int lineno, size_t len,
 		     int sockfd, struct msghdr *msg, int flags)
 {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library
  2021-04-26 11:19 [LTP] [PATCH 0/4] RTNetlink and network device management library Martin Doucha
  2021-04-26 11:19 ` [LTP] [PATCH 1/4] Add SAFE_REALLOC() helper function to LTP library Martin Doucha
  2021-04-26 11:19 ` [LTP] [PATCH 2/4] Add SAFE_RECV() " Martin Doucha
@ 2021-04-26 11:19 ` Martin Doucha
  2021-04-27 13:41   ` Cyril Hrubis
  2021-04-27 15:44   ` Cyril Hrubis
  2021-04-26 11:19 ` [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces Martin Doucha
  3 siblings, 2 replies; 14+ messages in thread
From: Martin Doucha @ 2021-04-26 11:19 UTC (permalink / raw)
  To: ltp

This library provides simple interface for creating arbitrary rtnetlink
messages with complex attributes, sending requests and receiving results.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 include/tst_rtnetlink.h | 105 +++++++++++
 lib/tst_rtnetlink.c     | 399 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 504 insertions(+)
 create mode 100644 include/tst_rtnetlink.h
 create mode 100644 lib/tst_rtnetlink.c

diff --git a/include/tst_rtnetlink.h b/include/tst_rtnetlink.h
new file mode 100644
index 000000000..3b307fbec
--- /dev/null
+++ b/include/tst_rtnetlink.h
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (c) 2021 Linux Test Project
+ */
+
+#ifndef TST_RTNETLINK_H
+#define TST_RTNETLINK_H
+
+struct tst_rtnl_context;
+
+struct tst_rtnl_attr_list {
+	unsigned short type;
+	const void *data;
+	ssize_t len;
+	const struct tst_rtnl_attr_list *sublist;
+};
+
+struct tst_rtnl_message {
+	struct nlmsghdr *header;
+	struct nlmsgerr *err;
+	void *payload;
+	size_t payload_size;
+};
+
+/* Open a netlink socket */
+struct tst_rtnl_context *tst_rtnl_create_context(const char *file,
+	const int lineno);
+#define RTNL_CREATE_CONTEXT() tst_rtnl_create_context(__FILE__, __LINE__)
+
+/* Free a tst_rtnl_message array returned by tst_rtnl_recv() */
+void tst_rtnl_free_message(struct tst_rtnl_message *msg);
+#define RTNL_FREE_MESSAGE tst_rtnl_free_message
+
+/* Close netlink socket */
+void tst_rtnl_free_context(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx);
+#define RTNL_FREE_CONTEXT(ctx) tst_rtnl_free_context(__FILE__, __LINE__, (ctx))
+
+/* Send all messages in given buffer */
+int tst_rtnl_send(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx);
+#define RTNL_SEND(ctx) tst_rtnl_send(__FILE__, __LINE__, (ctx))
+
+/* Send all messages in given buffer and validate kernel response */
+int tst_rtnl_send_validate(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx);
+#define RTNL_SEND_VALIDATE(ctx) \
+	tst_rtnl_send_validate(__FILE__, __LINE__, (ctx))
+
+/* Wait until data is available for reading from the netlink socket */
+int tst_rtnl_wait(struct tst_rtnl_context *ctx);
+#define RTNL_WAIT tst_rtnl_wait
+
+/*
+ * Read from netlink socket and return an array of partially parsed messages.
+ * header == NULL indicates end of array.
+ */
+struct tst_rtnl_message *tst_rtnl_recv(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx);
+#define RTNL_RECV(ctx) tst_rtnl_recv(__FILE__, __LINE__, (ctx))
+
+/* Add new message to buffer */
+int tst_rtnl_add_message(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, const struct nlmsghdr *header,
+	const void *payload, size_t payload_size);
+#define RTNL_ADD_MESSAGE(ctx, header, payload, psize) \
+	tst_rtnl_add_message(__FILE__, __LINE__, (ctx), (header), (payload), \
+		(psize))
+
+/* Add arbitrary attribute to last message */
+int tst_rtnl_add_attr(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, unsigned short type, const void *data,
+	unsigned short len);
+#define RTNL_ADD_ATTR(ctx, type, data, len) \
+	tst_rtnl_add_attr(__FILE__, __LINE__, (ctx), (type), (data), (len))
+
+/* Add string attribute to last message */
+int tst_rtnl_add_attr_string(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, unsigned short type, const char *data);
+#define RTNL_ADD_ATTR_STRING(ctx, type, data) \
+	tst_rtnl_add_attr_string(__FILE__, __LINE__, (ctx), (type), (data))
+
+/*
+ * Add list of arbitrary attributes to last message. The list is terminated
+ * by attribute with negative length. Nested sublists are supported.
+ */
+int tst_rtnl_add_attr_list(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, const struct tst_rtnl_attr_list *list);
+#define RTNL_ADD_ATTR_LIST(ctx, list) \
+	tst_rtnl_add_attr_list(__FILE__, __LINE__, (ctx), (list))
+
+/* Check that all sent messages with NLM_F_ACK flag have been acked without
+ * error. Usage:
+ *
+ * tst_rtnl_send(ctx);
+ * tst_rtnl_wait(ctx);
+ * response = tst_rtnl_recv(ctx);
+ * if (!tst_rtnl_check_acks(ctx, response)) { ... }
+ * tst_rtnl_free_message(response);
+ */
+int tst_rtnl_check_acks(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, struct tst_rtnl_message *response);
+#define RTNL_CHECK_ACKS(ctx, response) \
+	tst_rtnl_context(__FILE__, __LINE__, (ctx), (response))
+
+#endif /* TST_RTNETLINK_H */
diff --git a/lib/tst_rtnetlink.c b/lib/tst_rtnetlink.c
new file mode 100644
index 000000000..bb6116d57
--- /dev/null
+++ b/lib/tst_rtnetlink.c
@@ -0,0 +1,399 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Linux Test Project
+ */
+
+#include <stdlib.h>
+#include <limits.h>
+#include <asm/types.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/select.h>
+#define TST_NO_DEFAULT_MAIN
+#include "tst_test.h"
+#include "tst_rtnetlink.h"
+
+struct tst_rtnl_context {
+	int socket;
+	pid_t pid;
+	uint32_t seq;
+	size_t bufsize, datalen;
+	char *buffer;
+	struct nlmsghdr *curmsg;
+};
+
+static int tst_rtnl_grow_buffer(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, size_t size)
+{
+	size_t needed, offset, curlen = NLMSG_ALIGN(ctx->datalen);
+	char *buf;
+
+	if (ctx->bufsize - curlen >= size)
+		return 1;
+
+	needed = size - (ctx->bufsize - curlen);
+	size = ctx->bufsize + (ctx->bufsize > needed ? ctx->bufsize : needed);
+	size = NLMSG_ALIGN(size);
+	buf = safe_realloc(file, lineno, ctx->buffer, size);
+
+	if (!buf)
+		return 0;
+
+	memset(buf + ctx->bufsize, 0, size - ctx->bufsize);
+	offset = ((char *)ctx->curmsg) - ctx->buffer;
+	ctx->buffer = buf;
+	ctx->curmsg = (struct nlmsghdr *)(buf + offset);
+	ctx->bufsize = size;
+	return 1;
+}
+
+struct tst_rtnl_context *tst_rtnl_create_context(const char *file,
+	const int lineno)
+{
+	struct tst_rtnl_context *ctx;
+	struct sockaddr_nl addr = {0};
+
+	ctx = safe_malloc(file, lineno, NULL, sizeof(struct tst_rtnl_context));
+
+	if (!ctx)
+		return NULL;
+
+	ctx->pid = 0;
+	ctx->seq = 0;
+	ctx->bufsize = 1024;
+	ctx->datalen = 0;
+	ctx->curmsg = NULL;
+	ctx->socket = safe_socket(file, lineno, NULL, AF_NETLINK,
+		SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_ROUTE);
+	addr.nl_family = AF_NETLINK;
+
+	if (ctx->socket < 0) {
+		free(ctx);
+		return NULL;
+	}
+
+	if (safe_bind(file, lineno, NULL, ctx->socket, (struct sockaddr *)&addr,
+		sizeof(addr))) {
+		free(ctx);
+		return NULL;
+	}
+
+	ctx->buffer = safe_malloc(file, lineno, NULL, ctx->bufsize);
+
+	if (!ctx->buffer) {
+		safe_close(file, lineno, NULL, ctx->socket);
+		free(ctx);
+		return NULL;
+	}
+
+	memset(ctx->buffer, 0, ctx->bufsize);
+	return ctx;
+}
+
+void tst_rtnl_free_message(struct tst_rtnl_message *msg)
+{
+	if (!msg)
+		return;
+
+	// all ptr->header and ptr->info pointers point to the same buffer
+	// msg->header is the start of the buffer
+	free(msg->header);
+	free(msg);
+}
+
+void tst_rtnl_free_context(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx)
+{
+	safe_close(file, lineno, NULL, ctx->socket);
+	free(ctx->buffer);
+	free(ctx);
+}
+
+int tst_rtnl_send(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx)
+{
+	struct sockaddr_nl addr = {0};
+	struct iovec iov;
+	struct msghdr msg = {0};
+	int ret;
+
+	if (!ctx->curmsg) {
+		tst_brk_(file, lineno, TBROK, "%s(): No message to send",
+			__func__);
+		return 0;
+	}
+
+	if (ctx->curmsg->nlmsg_flags & NLM_F_MULTI) {
+		size_t size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
+
+		if (!tst_rtnl_grow_buffer(file, lineno, ctx, NLMSG_SPACE(0)))
+			return 0;
+
+		ctx->curmsg = NLMSG_NEXT(ctx->curmsg, size);
+		memset(ctx->curmsg, 0, sizeof(struct nlmsghdr));
+		ctx->curmsg->nlmsg_len = NLMSG_LENGTH(0);
+		ctx->curmsg->nlmsg_type = NLMSG_DONE;
+		ctx->curmsg->nlmsg_flags = 0;
+		ctx->curmsg->nlmsg_seq = ctx->seq++;
+		ctx->curmsg->nlmsg_pid = ctx->pid;
+		ctx->datalen = NLMSG_ALIGN(ctx->datalen) + NLMSG_LENGTH(0);
+	}
+
+	addr.nl_family = AF_NETLINK;
+	iov.iov_base = ctx->buffer;
+	iov.iov_len = ctx->datalen;
+	msg.msg_name = &addr;
+	msg.msg_namelen = sizeof(addr);
+	msg.msg_iov = &iov;
+	msg.msg_iovlen = 1;
+
+	ret = safe_sendmsg(file, lineno, ctx->datalen, ctx->socket, &msg, 0);
+
+	if (ret > 0)
+		ctx->curmsg = NULL;
+
+	return ret;
+}
+
+int tst_rtnl_wait(struct tst_rtnl_context *ctx)
+{
+	fd_set fdlist;
+	struct timeval timeout = {0};
+
+	FD_ZERO(&fdlist);
+	FD_SET(ctx->socket, &fdlist);
+	timeout.tv_sec = 1;
+
+	return select(ctx->socket + 1, &fdlist, NULL, NULL, &timeout);
+}
+
+struct tst_rtnl_message *tst_rtnl_recv(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx)
+{
+	char *buffer, tmp;
+	struct tst_rtnl_message *ret;
+	struct nlmsghdr *ptr;
+	ssize_t size;
+	int i, size_left, msgcount;
+
+	errno = 0;
+	size = recv(ctx->socket, &tmp, 1, MSG_DONTWAIT | MSG_PEEK | MSG_TRUNC);
+
+	if (size <= 0) {
+		if (errno != EAGAIN)
+			tst_brk_(file, lineno, TBROK | TERRNO, "recv() failed");
+		return NULL;
+	}
+
+	buffer = safe_malloc(file, lineno, NULL, size);
+
+	if (!buffer)
+		return NULL;
+
+	size = safe_recv(file, lineno, size, ctx->socket, buffer, size, 0);
+
+	if (size <= 0) {
+		free(buffer);
+		return NULL;
+	}
+
+	ptr = (struct nlmsghdr *)buffer;
+	size_left = size;
+	msgcount = 0;
+
+	for (; size_left > 0 && NLMSG_OK(ptr, size_left); msgcount++)
+		ptr = NLMSG_NEXT(ptr, size_left);
+
+	ret = safe_malloc(file, lineno, NULL,
+		(msgcount + 1) * sizeof(struct tst_rtnl_message));
+
+	if (!ret) {
+		free(buffer);
+		return NULL;
+	}
+
+	memset(ret, 0, (msgcount + 1) * sizeof(struct tst_rtnl_message));
+	ptr = (struct nlmsghdr *)buffer;
+	size_left = size;
+
+	for (i = 0; i < msgcount; i++, ptr = NLMSG_NEXT(ptr, size_left)) {
+		ret[i].header = ptr;
+		ret[i].payload = NLMSG_DATA(ptr);
+		ret[i].payload_size = NLMSG_PAYLOAD(ptr, 0);
+
+		if (ptr->nlmsg_type == NLMSG_ERROR)
+			ret[i].err = NLMSG_DATA(ptr);
+	}
+
+	return ret;
+}
+
+int tst_rtnl_add_message(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, const struct nlmsghdr *header,
+	const void *payload, size_t payload_size)
+{
+	size_t size;
+	unsigned int extra_flags = 0;
+
+	if (!tst_rtnl_grow_buffer(file, lineno, ctx, NLMSG_SPACE(payload_size)))
+		return 0;
+
+	if (!ctx->curmsg) {
+		/*
+		 * datalen may hold the size of last sent message for ACK
+		 * checking, reset it back to 0 here
+		 */
+		ctx->datalen = 0;
+		ctx->curmsg = (struct nlmsghdr *)ctx->buffer;
+	} else {
+		size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
+
+		extra_flags = NLM_F_MULTI;
+		ctx->curmsg->nlmsg_flags |= extra_flags;
+		ctx->curmsg = NLMSG_NEXT(ctx->curmsg, size);
+		ctx->datalen = NLMSG_ALIGN(ctx->datalen);
+	}
+
+	*ctx->curmsg = *header;
+	ctx->curmsg->nlmsg_len = NLMSG_LENGTH(payload_size);
+	ctx->curmsg->nlmsg_flags |= extra_flags;
+	ctx->curmsg->nlmsg_seq = ctx->seq++;
+	ctx->curmsg->nlmsg_pid = ctx->pid;
+	memcpy(NLMSG_DATA(ctx->curmsg), payload, payload_size);
+	ctx->datalen += ctx->curmsg->nlmsg_len;
+	return 1;
+}
+
+int tst_rtnl_add_attr(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, unsigned short type,
+	const void *data, unsigned short len)
+{
+	size_t size;
+	struct rtattr *attr;
+
+	if (!ctx->curmsg) {
+		tst_brk_(file, lineno, TBROK,
+			"%s(): No message to add attributes to", __func__);
+		return 0;
+	}
+
+	if (!tst_rtnl_grow_buffer(file, lineno, ctx, RTA_SPACE(len)))
+		return 0;
+
+	size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
+	attr = (struct rtattr *)(((char *)ctx->curmsg) + size);
+	attr->rta_type = type;
+	attr->rta_len = RTA_LENGTH(len);
+	memcpy(RTA_DATA(attr), data, len);
+	ctx->curmsg->nlmsg_len = size + attr->rta_len;
+	ctx->datalen = NLMSG_ALIGN(ctx->datalen) + attr->rta_len;
+	return 1;
+}
+
+int tst_rtnl_add_attr_string(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, unsigned short type,
+	const char *data)
+{
+	return tst_rtnl_add_attr(file, lineno, ctx, type, data,
+		strlen(data) + 1);
+}
+
+int tst_rtnl_add_attr_list(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx,
+	const struct tst_rtnl_attr_list *list)
+{
+	int i, ret;
+	size_t offset;
+
+	for (i = 0; list[i].len >= 0; i++) {
+		if (list[i].len > USHRT_MAX) {
+			tst_brk_(file, lineno, TBROK,
+				"%s(): Attribute value too long", __func__);
+			return -1;
+		}
+
+		offset = NLMSG_ALIGN(ctx->datalen);
+		ret = tst_rtnl_add_attr(file, lineno, ctx, list[i].type,
+			list[i].data, list[i].len);
+
+		if (!ret)
+			return -1;
+
+		if (list[i].sublist) {
+			struct rtattr *attr;
+
+			ret = tst_rtnl_add_attr_list(file, lineno, ctx,
+				list[i].sublist);
+
+			if (ret < 0)
+				return ret;
+
+			attr = (struct rtattr *)(ctx->buffer + offset);
+
+			if (ctx->datalen - offset > USHRT_MAX) {
+				tst_brk_(file, lineno, TBROK,
+					"%s(): Sublist too long", __func__);
+				return -1;
+			}
+
+			attr->rta_len = ctx->datalen - offset;
+		}
+	}
+
+	return i;
+}
+
+int tst_rtnl_check_acks(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx, struct tst_rtnl_message *res)
+{
+	struct nlmsghdr *msg = (struct nlmsghdr *)ctx->buffer;
+	int size_left = ctx->datalen;
+
+	for (; size_left > 0 && NLMSG_OK(msg, size_left);
+		msg = NLMSG_NEXT(msg, size_left)) {
+
+		if (!(msg->nlmsg_flags & NLM_F_ACK))
+			continue;
+
+		while (res->header && res->header->nlmsg_seq != msg->nlmsg_seq)
+			res++;
+
+		if (!res->err || res->header->nlmsg_seq != msg->nlmsg_seq) {
+			tst_brk_(file, lineno, TBROK,
+				"No ACK found for Netlink message %u",
+				msg->nlmsg_seq);
+			return 0;
+		}
+
+		if (res->err->error) {
+			TST_ERR = -res->err->error;
+			return 0;
+		}
+	}
+
+	return 1;
+}
+
+int tst_rtnl_send_validate(const char *file, const int lineno,
+	struct tst_rtnl_context *ctx)
+{
+	struct tst_rtnl_message *response;
+	int ret;
+
+	TST_ERR = 0;
+
+	if (tst_rtnl_send(file, lineno, ctx) <= 0)
+		return 0;
+
+	tst_rtnl_wait(ctx);
+	response = tst_rtnl_recv(file, lineno, ctx);
+
+	if (!response)
+		return 0;
+
+	ret = tst_rtnl_check_acks(file, lineno, ctx, response);
+	tst_rtnl_free_message(response);
+	return ret;
+}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces
  2021-04-26 11:19 [LTP] [PATCH 0/4] RTNetlink and network device management library Martin Doucha
                   ` (2 preceding siblings ...)
  2021-04-26 11:19 ` [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library Martin Doucha
@ 2021-04-26 11:19 ` Martin Doucha
  2021-04-28 10:27   ` Cyril Hrubis
  3 siblings, 1 reply; 14+ messages in thread
From: Martin Doucha @ 2021-04-26 11:19 UTC (permalink / raw)
  To: ltp

The library currently supports:
- creating a virtual ethernet device pair
- removing network interfaces
- enabling or disabling a network interface
- managing interface addresses
- managing routing table entries
- moving network interfaces between network namespaces

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 include/tst_netdevice.h | 120 ++++++++++
 lib/tst_netdevice.c     | 506 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 626 insertions(+)
 create mode 100644 include/tst_netdevice.h
 create mode 100644 lib/tst_netdevice.c

diff --git a/include/tst_netdevice.h b/include/tst_netdevice.h
new file mode 100644
index 000000000..69f559fdd
--- /dev/null
+++ b/include/tst_netdevice.h
@@ -0,0 +1,120 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (c) 2021 Linux Test Project
+ */
+
+#ifndef TST_NETDEVICE_H
+#define TST_NETDEVICE_H
+
+/* Find device index for given network interface name. */
+int tst_netdevice_index(const char *file, const int lineno, const char *ifname);
+#define NETDEVICE_INDEX(ifname) \
+	tst_netdevice_index(__FILE__, __LINE__, (ifname))
+
+/* Activate or deactivate network interface */
+int tst_netdevice_activate(const char *file, const int lineno,
+	const char *ifname, int up);
+#define NETDEVICE_ACTIVATE(ifname, up) \
+	tst_netdevice_activate(__FILE__, __LINE__, (ifname), (up))
+
+/* Create a connected pair of virtual network devices */
+int tst_create_veth_pair(const char *file, const int lineno,
+	const char *ifname1, const char *ifname2);
+#define CREATE_VETH_PAIR(ifname1, ifname2) \
+	tst_create_veth_pair(__FILE__, __LINE__, (ifname1), (ifname2))
+
+int tst_remove_netdevice(const char *file, const int lineno,
+	const char *ifname);
+#define REMOVE_NETDEVICE(ifname) \
+	tst_remove_netdevice(__FILE__, __LINE__, (ifname))
+
+int tst_netdevice_add_address(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *address,
+	unsigned int prefix, size_t addrlen, unsigned int flags);
+#define NETDEVICE_ADD_ADDRESS(ifname, family, address, prefix, addrlen, flags) \
+	tst_netdevice_add_address(__FILE__, __LINE__, (ifname), (family), \
+		(address), (prefix), (addrlen), (flags))
+
+int tst_netdevice_add_address_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t address, unsigned int prefix,
+	unsigned int flags);
+#define NETDEVICE_ADD_ADDRESS_INET(ifname, address, prefix, flags) \
+	tst_netdevice_add_address_inet(__FILE__, __LINE__, (ifname), \
+		(address), (prefix), (flags))
+
+int tst_netdevice_remove_address(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *address,
+	size_t addrlen);
+#define NETDEVICE_REMOVE_ADDRESS(ifname, family, address, addrlen) \
+	tst_netdevice_remove_address(__FILE__, __LINE__, (ifname), (family), \
+		(address), (addrlen))
+
+int tst_netdevice_remove_address_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t address);
+#define NETDEVICE_REMOVE_ADDRESS_INET(ifname, address) \
+	tst_netdevice_remove_address_inet(__FILE__, __LINE__, (ifname), \
+		(address))
+
+int tst_netdevice_change_ns_fd(const char *file, const int lineno,
+	const char *ifname, int nsfd);
+#define NETDEVICE_CHANGE_NS_FD(ifname, nsfd) \
+	tst_netdevice_change_ns_fd(__FILE__, __LINE__, (ifname), (nsfd))
+
+int tst_netdevice_change_ns_pid(const char *file, const int lineno,
+	const char *ifname, pid_t nspid);
+#define NETDEVICE_CHANGE_NS_PID(ifname, nspid) \
+	tst_netdevice_change_ns_pid(__FILE__, __LINE__, (ifname), (nspid))
+
+/*
+ * Add new static entry to main routing table. If you specify gateway address,
+ * the interface name is optional.
+ */
+int tst_netdevice_add_route(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *srcaddr,
+	unsigned int srcprefix, size_t srclen, const void *dstaddr,
+	unsigned int dstprefix, size_t dstlen, const void *gateway,
+	size_t gatewaylen);
+#define NETDEVICE_ADD_ROUTE(ifname, family, srcaddr, srcprefix, srclen, \
+	dstaddr, dstprefix, dstlen, gateway, gatewaylen) \
+	tst_netdevice_add_route(__FILE__, __LINE__, (ifname), (family), \
+		(srcaddr), (srcprefix), (srclen), (dstaddr), (dstprefix), \
+		(dstlen), (gateway), (gatewaylen))
+
+/*
+ * Simplified function for adding IPv4 static route. If you set srcprefix
+ * or dstprefix to 0, the corresponding address will be ignored. Interface
+ * name is optional if gateway address is non-zero.
+ */
+int tst_netdevice_add_route_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
+	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway);
+#define NETDEVICE_ADD_ROUTE_INET(ifname, srcaddr, srcprefix, dstaddr, \
+	dstprefix, gateway) \
+	tst_netdevice_add_route_inet(__FILE__, __LINE__, (ifname), (srcaddr), \
+		(srcprefix), (dstaddr), (dstprefix), (gateway))
+
+/*
+ * Remove static entry from main routing table.
+ */
+int tst_netdevice_remove_route(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *srcaddr,
+	unsigned int srcprefix, size_t srclen, const void *dstaddr,
+	unsigned int dstprefix, size_t dstlen, const void *gateway,
+	size_t gatewaylen);
+#define NETDEVICE_REMOVE_ROUTE(ifname, family, srcaddr, srcprefix, srclen, \
+	dstaddr, dstprefix, dstlen, gateway, gatewaylen) \
+	tst_netdevice_remove_route(__FILE__, __LINE__, (ifname), (family), \
+		(srcaddr), (srcprefix), (srclen), (dstaddr), (dstprefix), \
+		(dstlen), (gateway), (gatewaylen))
+
+/*
+ * Simplified function for removing IPv4 static route.
+ */
+int tst_netdevice_remove_route_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
+	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway);
+#define NETDEVICE_REMOVE_ROUTE_INET(ifname, srcaddr, srcprefix, dstaddr, \
+	dstprefix, gateway) \
+	tst_netdevice_remove_route_inet(__FILE__, __LINE__, (ifname), \
+		(srcaddr), (srcprefix), (dstaddr), (dstprefix), (gateway))
+
+#endif /* TST_NETDEVICE_H */
diff --git a/lib/tst_netdevice.c b/lib/tst_netdevice.c
new file mode 100644
index 000000000..541541b11
--- /dev/null
+++ b/lib/tst_netdevice.c
@@ -0,0 +1,506 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Linux Test Project
+ */
+
+#include <asm/types.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <linux/veth.h>
+#include <sys/socket.h>
+#include <net/if.h>
+#define TST_NO_DEFAULT_MAIN
+#include "tst_test.h"
+#include "tst_rtnetlink.h"
+#include "tst_netdevice.h"
+
+static struct tst_rtnl_context *create_request(const char *file,
+	const int lineno, unsigned int type, unsigned int flags,
+	const void *payload, size_t psize)
+{
+	struct nlmsghdr header = {0};
+	struct tst_rtnl_context *ctx;
+
+	ctx = tst_rtnl_create_context(file, lineno);
+
+	if (!ctx)
+		return NULL;
+
+	header.nlmsg_type = type;
+	header.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | flags;
+
+	if (!tst_rtnl_add_message(file, lineno, ctx, &header, payload, psize)) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return NULL;
+	}
+
+	return ctx;
+}
+
+int tst_netdevice_index(const char *file, const int lineno, const char *ifname)
+{
+	struct ifreq ifr;
+	int sock;
+
+	if (strlen(ifname) >= IFNAMSIZ) {
+		tst_brk_(file, lineno, TBROK,
+			"Network device name \"%s\" too long", ifname);
+		return -1;
+	}
+
+	sock = safe_socket(file, lineno, NULL, AF_INET, SOCK_DGRAM, 0);
+
+	if (sock < 0)
+		return -1;
+
+	strcpy(ifr.ifr_name, ifname);
+	TEST(ioctl(sock, SIOCGIFINDEX, &ifr));
+	safe_close(file, lineno, NULL, sock);
+
+	if (TST_RET < 0) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"ioctl(SIOCGIFINDEX) failed");
+	} else if (TST_RET) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Invalid ioctl(SIOCGIFINDEX) return value %ld",
+			TST_RET);
+	}
+
+	return TST_RET ? -1 : ifr.ifr_ifindex;
+}
+
+int tst_netdevice_activate(const char *file, const int lineno,
+	const char *ifname, int up)
+{
+	struct ifreq ifr;
+	int sock;
+
+	if (strlen(ifname) >= IFNAMSIZ) {
+		tst_brk_(file, lineno, TBROK,
+			"Network device name \"%s\" too long", ifname);
+		return -1;
+	}
+
+	sock = safe_socket(file, lineno, NULL, AF_INET, SOCK_DGRAM, 0);
+
+	if (sock < 0)
+		return -1;
+
+	strcpy(ifr.ifr_name, ifname);
+	TEST(ioctl(sock, SIOCGIFFLAGS, &ifr));
+
+	if (TST_RET < 0) {
+		safe_close(file, lineno, NULL, sock);
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"ioctl(SIOCGIFFLAGS) failed");
+		return TST_RET;
+	}
+
+	if (TST_RET) {
+		safe_close(file, lineno, NULL, sock);
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Invalid ioctl(SIOCGIFFLAGS) return value %ld",
+			TST_RET);
+		return TST_RET;
+	}
+
+	if (up)
+		ifr.ifr_flags |= IFF_UP;
+	else
+		ifr.ifr_flags &= ~IFF_UP;
+
+	TEST(ioctl(sock, SIOCSIFFLAGS, &ifr));
+	safe_close(file, lineno, NULL, sock);
+
+	if (TST_RET < 0) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"ioctl(SIOCSIFFLAGS) failed");
+	} else if (TST_RET) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Invalid ioctl(SIOCSIFFLAGS) return value %ld",
+			TST_RET);
+	}
+
+	return TST_RET;
+}
+
+int tst_create_veth_pair(const char *file, const int lineno,
+	const char *ifname1, const char *ifname2)
+{
+	int ret;
+	struct ifinfomsg info = {0};
+	struct tst_rtnl_context *ctx;
+	struct tst_rtnl_attr_list peerinfo[] = {
+		{IFLA_IFNAME, ifname2, strlen(ifname2) + 1, NULL},
+		{0, NULL, -1, NULL}
+	};
+	struct tst_rtnl_attr_list peerdata[] = {
+		{VETH_INFO_PEER, &info, sizeof(info), peerinfo},
+		{0, NULL, -1, NULL}
+	};
+	struct tst_rtnl_attr_list attrs[] = {
+		{IFLA_IFNAME, ifname1, strlen(ifname1) + 1, NULL},
+		{IFLA_LINKINFO, NULL, 0, (const struct tst_rtnl_attr_list[]){
+			{IFLA_INFO_KIND, "veth", 4, NULL},
+			{IFLA_INFO_DATA, NULL, 0, peerdata},
+			{0, NULL, -1, NULL}
+		}},
+		{0, NULL, -1, NULL}
+	};
+
+	if (strlen(ifname1) >= IFNAMSIZ) {
+		tst_brk_(file, lineno, TBROK,
+			"Network device name \"%s\" too long", ifname1);
+		return 0;
+	}
+
+	if (strlen(ifname2) >= IFNAMSIZ) {
+		tst_brk_(file, lineno, TBROK,
+			"Network device name \"%s\" too long", ifname2);
+		return 0;
+	}
+
+	info.ifi_family = AF_UNSPEC;
+	ctx = create_request(file, lineno, RTM_NEWLINK,
+		NLM_F_CREATE | NLM_F_EXCL, &info, sizeof(info));
+
+	if (!ctx)
+		return 0;
+
+	if (tst_rtnl_add_attr_list(file, lineno, ctx, attrs) != 2) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	tst_rtnl_free_context(file, lineno, ctx);
+
+	if (!ret) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Failed to create veth interfaces %s+%s", ifname1,
+			ifname2);
+	}
+
+	return ret;
+}
+
+int tst_remove_netdevice(const char *file, const int lineno, const char *ifname)
+{
+	struct ifinfomsg info = {0};
+	struct tst_rtnl_context *ctx;
+	int ret;
+
+	if (strlen(ifname) >= IFNAMSIZ) {
+		tst_brk_(file, lineno, TBROK,
+			"Network device name \"%s\" too long", ifname);
+		return 0;
+	}
+
+	info.ifi_family = AF_UNSPEC;
+	ctx = create_request(file, lineno, RTM_DELLINK, 0, &info, sizeof(info));
+
+	if (!ctx)
+		return 0;
+
+	if (!tst_rtnl_add_attr_string(file, lineno, ctx, IFLA_IFNAME, ifname)) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	tst_rtnl_free_context(file, lineno, ctx);
+
+	if (!ret) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Failed to remove netdevice %s", ifname);
+	}
+
+	return ret;
+}
+
+static int modify_address(const char *file, const int lineno,
+	unsigned int action, unsigned int nl_flags, const char *ifname,
+	unsigned int family, const void *address, unsigned int prefix,
+	size_t addrlen, uint32_t addr_flags)
+{
+	struct ifaddrmsg info = {0};
+	struct tst_rtnl_context *ctx;
+	int index, ret;
+
+	index = tst_netdevice_index(file, lineno, ifname);
+
+	if (index < 0) {
+		tst_brk_(file, lineno, TBROK, "Interface %s not found", ifname);
+		return 0;
+	}
+
+	info.ifa_family = family;
+	info.ifa_prefixlen = prefix;
+	info.ifa_index = index;
+	ctx = create_request(file, lineno, action, nl_flags, &info,
+		sizeof(info));
+
+	if (!ctx)
+		return 0;
+
+
+	if (!tst_rtnl_add_attr(file, lineno, ctx, IFA_FLAGS, &addr_flags,
+		sizeof(uint32_t))) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	if (!tst_rtnl_add_attr(file, lineno, ctx, IFA_LOCAL, address,
+		addrlen)) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	tst_rtnl_free_context(file, lineno, ctx);
+
+	if (!ret) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Failed to modify %s network address", ifname);
+	}
+
+	return ret;
+}
+
+int tst_netdevice_add_address(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *address,
+	unsigned int prefix, size_t addrlen, unsigned int flags)
+{
+	return modify_address(file, lineno, RTM_NEWADDR,
+		NLM_F_CREATE | NLM_F_EXCL, ifname, family, address, prefix,
+		addrlen, flags);
+}
+
+int tst_netdevice_add_address_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t address, unsigned int prefix,
+	unsigned int flags)
+{
+	return tst_netdevice_add_address(file, lineno, ifname, AF_INET,
+		&address, prefix, sizeof(address), flags);
+}
+
+int tst_netdevice_remove_address(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *address,
+	size_t addrlen)
+{
+	return modify_address(file, lineno, RTM_DELADDR, 0, ifname, family,
+		address, 0, addrlen, 0);
+}
+
+int tst_netdevice_remove_address_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t address)
+{
+	return tst_netdevice_remove_address(file, lineno, ifname, AF_INET,
+		&address, sizeof(address));
+}
+
+static int change_ns(const char *file, const int lineno, const char *ifname,
+	unsigned short attr, uint32_t value)
+{
+	struct ifinfomsg info = {0};
+	struct tst_rtnl_context *ctx;
+	int ret;
+
+	if (strlen(ifname) >= IFNAMSIZ) {
+		tst_brk_(file, lineno, TBROK,
+			"Network device name \"%s\" too long", ifname);
+		return 0;
+	}
+
+	info.ifi_family = AF_UNSPEC;
+	ctx = create_request(file, lineno, RTM_NEWLINK, 0, &info, sizeof(info));
+
+	if (!tst_rtnl_add_attr_string(file, lineno, ctx, IFLA_IFNAME, ifname)) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	if (!tst_rtnl_add_attr(file, lineno, ctx, attr, &value,
+		sizeof(uint32_t))) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	tst_rtnl_free_context(file, lineno, ctx);
+
+	if (!ret) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Failed to move %s to another namespace", ifname);
+	}
+
+	return ret;
+}
+
+int tst_netdevice_change_ns_fd(const char *file, const int lineno,
+	const char *ifname, int nsfd)
+{
+	return change_ns(file, lineno, ifname, IFLA_NET_NS_FD, nsfd);
+}
+
+int tst_netdevice_change_ns_pid(const char *file, const int lineno,
+	const char *ifname, pid_t nspid)
+{
+	return change_ns(file, lineno, ifname, IFLA_NET_NS_PID, nspid);
+}
+
+static int modify_route(const char *file, const int lineno, unsigned int action,
+	unsigned int flags, const char *ifname, unsigned int family,
+	const void *srcaddr, unsigned int srcprefix, size_t srclen,
+	const void *dstaddr, unsigned int dstprefix, size_t dstlen,
+	const void *gateway, size_t gatewaylen)
+{
+	struct rtmsg info = {0};
+	struct tst_rtnl_context *ctx;
+	int ret;
+	int32_t index;
+
+	if (!ifname && !gateway) {
+		tst_brk_(file, lineno, TBROK,
+			"Interface name or gateway address required");
+		return 0;
+	}
+
+	if (ifname && strlen(ifname) >= IFNAMSIZ) {
+		tst_brk_(file, lineno, TBROK,
+			"Network device name \"%s\" too long", ifname);
+		return 0;
+	}
+
+	if (ifname) {
+		index = tst_netdevice_index(file, lineno, ifname);
+
+		if (index < 0)
+			return 0;
+	}
+
+	info.rtm_family = family;
+	info.rtm_dst_len = dstprefix;
+	info.rtm_src_len = srcprefix;
+	info.rtm_table = RT_TABLE_MAIN;
+	info.rtm_protocol = RTPROT_STATIC;
+	info.rtm_type = RTN_UNICAST;
+
+	if (action == RTM_DELROUTE) {
+		tst_res_(file, lineno, TINFO, "DELROUTE");
+		info.rtm_scope = RT_SCOPE_NOWHERE;
+	} else {
+		tst_res_(file, lineno, TINFO, "ADDROUTE");
+		info.rtm_scope = RT_SCOPE_UNIVERSE;
+	}
+
+	ctx = create_request(file, lineno, action, flags, &info, sizeof(info));
+
+	if (srcaddr && !tst_rtnl_add_attr(file, lineno, ctx, RTA_SRC, srcaddr,
+		srclen)) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	if (dstaddr && !tst_rtnl_add_attr(file, lineno, ctx, RTA_DST, dstaddr,
+		dstlen)) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	if (gateway && !tst_rtnl_add_attr(file, lineno, ctx, RTA_GATEWAY,
+		gateway, gatewaylen)) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	if (ifname && !tst_rtnl_add_attr(file, lineno, ctx, RTA_OIF, &index,
+		sizeof(index))) {
+		tst_rtnl_free_context(file, lineno, ctx);
+		return 0;
+	}
+
+	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	tst_rtnl_free_context(file, lineno, ctx);
+
+	if (!ret) {
+		tst_brk_(file, lineno, TBROK | TTERRNO,
+			"Failed to modify network route");
+	}
+
+	return ret;
+}
+
+int tst_netdevice_add_route(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *srcaddr,
+	unsigned int srcprefix, size_t srclen, const void *dstaddr,
+	unsigned int dstprefix, size_t dstlen, const void *gateway,
+	size_t gatewaylen)
+{
+	return modify_route(file, lineno, RTM_NEWROUTE,
+		NLM_F_CREATE | NLM_F_EXCL, ifname, family, srcaddr, srcprefix,
+		srclen, dstaddr, dstprefix, dstlen, gateway, gatewaylen);
+}
+
+int tst_netdevice_add_route_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
+	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway)
+{
+	void *src = NULL, *dst = NULL, *gw = NULL;
+	size_t srclen = 0, dstlen = 0, gwlen = 0;
+
+	if (srcprefix) {
+		src = &srcaddr;
+		srclen = sizeof(srcaddr);
+	}
+
+	if (dstprefix) {
+		dst = &dstaddr;
+		dstlen = sizeof(dstaddr);
+	}
+
+	if (gateway) {
+		gw = &gateway;
+		gwlen = sizeof(gateway);
+	}
+
+	return tst_netdevice_add_route(file, lineno, ifname, AF_INET, src,
+		srcprefix, srclen, dst, dstprefix, dstlen, gw, gwlen);
+}
+
+int tst_netdevice_remove_route(const char *file, const int lineno,
+	const char *ifname, unsigned int family, const void *srcaddr,
+	unsigned int srcprefix, size_t srclen, const void *dstaddr,
+	unsigned int dstprefix, size_t dstlen, const void *gateway,
+	size_t gatewaylen)
+{
+	return modify_route(file, lineno, RTM_DELROUTE, 0, ifname, family,
+		srcaddr, srcprefix, srclen, dstaddr, dstprefix, dstlen,
+		gateway, gatewaylen);
+}
+
+int tst_netdevice_remove_route_inet(const char *file, const int lineno,
+	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
+	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway)
+{
+	void *src = NULL, *dst = NULL, *gw = NULL;
+	size_t srclen = 0, dstlen = 0, gwlen = 0;
+
+	if (srcprefix) {
+		src = &srcaddr;
+		srclen = sizeof(srcaddr);
+	}
+
+	if (dstprefix) {
+		dst = &dstaddr;
+		dstlen = sizeof(dstaddr);
+	}
+
+	if (gateway) {
+		gw = &gateway;
+		gwlen = sizeof(gateway);
+	}
+
+	return tst_netdevice_remove_route(file, lineno, ifname, AF_INET, src,
+		srcprefix, srclen, dst, dstprefix, dstlen, gw, gwlen);
+}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 1/4] Add SAFE_REALLOC() helper function to LTP library
  2021-04-26 11:19 ` [LTP] [PATCH 1/4] Add SAFE_REALLOC() helper function to LTP library Martin Doucha
@ 2021-04-27 13:04   ` Cyril Hrubis
  0 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2021-04-27 13:04 UTC (permalink / raw)
  To: ltp

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 2/4] Add SAFE_RECV() helper function to LTP library
  2021-04-26 11:19 ` [LTP] [PATCH 2/4] Add SAFE_RECV() " Martin Doucha
@ 2021-04-27 13:06   ` Cyril Hrubis
  0 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2021-04-27 13:06 UTC (permalink / raw)
  To: ltp

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library
  2021-04-26 11:19 ` [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library Martin Doucha
@ 2021-04-27 13:41   ` Cyril Hrubis
  2021-04-27 14:14     ` Martin Doucha
  2021-04-27 15:44   ` Cyril Hrubis
  1 sibling, 1 reply; 14+ messages in thread
From: Cyril Hrubis @ 2021-04-27 13:41 UTC (permalink / raw)
  To: ltp

Hi!
> +static int tst_rtnl_grow_buffer(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, size_t size)
> +{
> +	size_t needed, offset, curlen = NLMSG_ALIGN(ctx->datalen);
> +	char *buf;
> +
> +	if (ctx->bufsize - curlen >= size)
> +		return 1;
> +
> +	needed = size - (ctx->bufsize - curlen);
> +	size = ctx->bufsize + (ctx->bufsize > needed ? ctx->bufsize : needed);
> +	size = NLMSG_ALIGN(size);
> +	buf = safe_realloc(file, lineno, ctx->buffer, size);
> +
> +	if (!buf)
> +		return 0;

You are calling safe_realloc() here yet you check the return value. And
it's the same for safe_malloc(), safe_bind(), safe_socket() and a few
more in the code.

So either we get rid of the error checks and of the error
propagation or we avoid using safe_ variants.

Other than that the code looks sane but it's hard to review the API
without an example that would excersize it. What about adding something
simple in newlib_tests?

> +	memset(buf + ctx->bufsize, 0, size - ctx->bufsize);
> +	offset = ((char *)ctx->curmsg) - ctx->buffer;
> +	ctx->buffer = buf;
> +	ctx->curmsg = (struct nlmsghdr *)(buf + offset);
> +	ctx->bufsize = size;
> +	return 1;
> +}
> +
> +struct tst_rtnl_context *tst_rtnl_create_context(const char *file,
> +	const int lineno)
> +{
> +	struct tst_rtnl_context *ctx;
> +	struct sockaddr_nl addr = {0};
> +
> +	ctx = safe_malloc(file, lineno, NULL, sizeof(struct tst_rtnl_context));
> +
> +	if (!ctx)
> +		return NULL;
> +
> +	ctx->pid = 0;
> +	ctx->seq = 0;
> +	ctx->bufsize = 1024;
> +	ctx->datalen = 0;
> +	ctx->curmsg = NULL;
> +	ctx->socket = safe_socket(file, lineno, NULL, AF_NETLINK,
> +		SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_ROUTE);
> +	addr.nl_family = AF_NETLINK;
> +
> +	if (ctx->socket < 0) {
> +		free(ctx);
> +		return NULL;
> +	}
> +
> +	if (safe_bind(file, lineno, NULL, ctx->socket, (struct sockaddr *)&addr,
> +		sizeof(addr))) {
> +		free(ctx);
> +		return NULL;
> +	}
> +
> +	ctx->buffer = safe_malloc(file, lineno, NULL, ctx->bufsize);
> +
> +	if (!ctx->buffer) {
> +		safe_close(file, lineno, NULL, ctx->socket);
> +		free(ctx);
> +		return NULL;
> +	}
> +
> +	memset(ctx->buffer, 0, ctx->bufsize);
> +	return ctx;
> +}
> +
> +void tst_rtnl_free_message(struct tst_rtnl_message *msg)
> +{
> +	if (!msg)
> +		return;
> +
> +	// all ptr->header and ptr->info pointers point to the same buffer
> +	// msg->header is the start of the buffer
> +	free(msg->header);
> +	free(msg);
> +}
> +
> +void tst_rtnl_free_context(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)

This should be probably called destroy_context() but that is very minor.

> +{
> +	safe_close(file, lineno, NULL, ctx->socket);
> +	free(ctx->buffer);
> +	free(ctx);
> +}
> +
> +int tst_rtnl_send(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)
> +{
> +	struct sockaddr_nl addr = {0};
> +	struct iovec iov;
> +	struct msghdr msg = {0};
> +	int ret;
> +
> +	if (!ctx->curmsg) {
> +		tst_brk_(file, lineno, TBROK, "%s(): No message to send",
> +			__func__);
> +		return 0;
> +	}
> +
> +	if (ctx->curmsg->nlmsg_flags & NLM_F_MULTI) {
> +		size_t size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
> +
> +		if (!tst_rtnl_grow_buffer(file, lineno, ctx, NLMSG_SPACE(0)))
> +			return 0;
> +
> +		ctx->curmsg = NLMSG_NEXT(ctx->curmsg, size);
> +		memset(ctx->curmsg, 0, sizeof(struct nlmsghdr));
> +		ctx->curmsg->nlmsg_len = NLMSG_LENGTH(0);
> +		ctx->curmsg->nlmsg_type = NLMSG_DONE;
> +		ctx->curmsg->nlmsg_flags = 0;
> +		ctx->curmsg->nlmsg_seq = ctx->seq++;
> +		ctx->curmsg->nlmsg_pid = ctx->pid;
> +		ctx->datalen = NLMSG_ALIGN(ctx->datalen) + NLMSG_LENGTH(0);
> +	}
> +
> +	addr.nl_family = AF_NETLINK;
> +	iov.iov_base = ctx->buffer;
> +	iov.iov_len = ctx->datalen;
> +	msg.msg_name = &addr;
> +	msg.msg_namelen = sizeof(addr);
> +	msg.msg_iov = &iov;
> +	msg.msg_iovlen = 1;
> +
> +	ret = safe_sendmsg(file, lineno, ctx->datalen, ctx->socket, &msg, 0);
> +
> +	if (ret > 0)
> +		ctx->curmsg = NULL;
> +
> +	return ret;
> +}
> +
> +int tst_rtnl_wait(struct tst_rtnl_context *ctx)
> +{
> +	fd_set fdlist;
> +	struct timeval timeout = {0};
> +
> +	FD_ZERO(&fdlist);
> +	FD_SET(ctx->socket, &fdlist);
> +	timeout.tv_sec = 1;
> +
> +	return select(ctx->socket + 1, &fdlist, NULL, NULL, &timeout);

I find the poll() syscall to have a bit saner API than this.

> +}
> +
> +struct tst_rtnl_message *tst_rtnl_recv(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)
> +{
> +	char *buffer, tmp;
> +	struct tst_rtnl_message *ret;
> +	struct nlmsghdr *ptr;
> +	ssize_t size;
> +	int i, size_left, msgcount;
> +
> +	errno = 0;
> +	size = recv(ctx->socket, &tmp, 1, MSG_DONTWAIT | MSG_PEEK | MSG_TRUNC);
> +
> +	if (size <= 0) {
> +		if (errno != EAGAIN)
> +			tst_brk_(file, lineno, TBROK | TERRNO, "recv() failed");
> +		return NULL;
> +	}
> +
> +	buffer = safe_malloc(file, lineno, NULL, size);
> +
> +	if (!buffer)
> +		return NULL;
> +
> +	size = safe_recv(file, lineno, size, ctx->socket, buffer, size, 0);
> +
> +	if (size <= 0) {
> +		free(buffer);
> +		return NULL;
> +	}
> +
> +	ptr = (struct nlmsghdr *)buffer;
> +	size_left = size;
> +	msgcount = 0;
> +
> +	for (; size_left > 0 && NLMSG_OK(ptr, size_left); msgcount++)
> +		ptr = NLMSG_NEXT(ptr, size_left);
> +
> +	ret = safe_malloc(file, lineno, NULL,
> +		(msgcount + 1) * sizeof(struct tst_rtnl_message));
> +
> +	if (!ret) {
> +		free(buffer);
> +		return NULL;
> +	}
> +
> +	memset(ret, 0, (msgcount + 1) * sizeof(struct tst_rtnl_message));
> +	ptr = (struct nlmsghdr *)buffer;
> +	size_left = size;
> +
> +	for (i = 0; i < msgcount; i++, ptr = NLMSG_NEXT(ptr, size_left)) {
> +		ret[i].header = ptr;
> +		ret[i].payload = NLMSG_DATA(ptr);
> +		ret[i].payload_size = NLMSG_PAYLOAD(ptr, 0);
> +
> +		if (ptr->nlmsg_type == NLMSG_ERROR)
> +			ret[i].err = NLMSG_DATA(ptr);
> +	}
> +
> +	return ret;
> +}
> +
> +int tst_rtnl_add_message(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, const struct nlmsghdr *header,
> +	const void *payload, size_t payload_size)
> +{
> +	size_t size;
> +	unsigned int extra_flags = 0;
> +
> +	if (!tst_rtnl_grow_buffer(file, lineno, ctx, NLMSG_SPACE(payload_size)))
> +		return 0;
> +
> +	if (!ctx->curmsg) {
> +		/*
> +		 * datalen may hold the size of last sent message for ACK
> +		 * checking, reset it back to 0 here
> +		 */
> +		ctx->datalen = 0;
> +		ctx->curmsg = (struct nlmsghdr *)ctx->buffer;
> +	} else {
> +		size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
> +
> +		extra_flags = NLM_F_MULTI;
> +		ctx->curmsg->nlmsg_flags |= extra_flags;
> +		ctx->curmsg = NLMSG_NEXT(ctx->curmsg, size);
> +		ctx->datalen = NLMSG_ALIGN(ctx->datalen);
> +	}
> +
> +	*ctx->curmsg = *header;
> +	ctx->curmsg->nlmsg_len = NLMSG_LENGTH(payload_size);
> +	ctx->curmsg->nlmsg_flags |= extra_flags;
> +	ctx->curmsg->nlmsg_seq = ctx->seq++;
> +	ctx->curmsg->nlmsg_pid = ctx->pid;
> +	memcpy(NLMSG_DATA(ctx->curmsg), payload, payload_size);
> +	ctx->datalen += ctx->curmsg->nlmsg_len;
> +	return 1;
> +}
> +
> +int tst_rtnl_add_attr(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, unsigned short type,
> +	const void *data, unsigned short len)
> +{
> +	size_t size;
> +	struct rtattr *attr;
> +
> +	if (!ctx->curmsg) {
> +		tst_brk_(file, lineno, TBROK,
> +			"%s(): No message to add attributes to", __func__);
> +		return 0;
> +	}
> +
> +	if (!tst_rtnl_grow_buffer(file, lineno, ctx, RTA_SPACE(len)))
> +		return 0;
> +
> +	size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
> +	attr = (struct rtattr *)(((char *)ctx->curmsg) + size);
> +	attr->rta_type = type;
> +	attr->rta_len = RTA_LENGTH(len);
> +	memcpy(RTA_DATA(attr), data, len);
> +	ctx->curmsg->nlmsg_len = size + attr->rta_len;
> +	ctx->datalen = NLMSG_ALIGN(ctx->datalen) + attr->rta_len;
> +	return 1;
> +}
> +
> +int tst_rtnl_add_attr_string(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, unsigned short type,
> +	const char *data)
> +{
> +	return tst_rtnl_add_attr(file, lineno, ctx, type, data,
> +		strlen(data) + 1);
> +}
> +
> +int tst_rtnl_add_attr_list(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx,
> +	const struct tst_rtnl_attr_list *list)
> +{
> +	int i, ret;
> +	size_t offset;
> +
> +	for (i = 0; list[i].len >= 0; i++) {
> +		if (list[i].len > USHRT_MAX) {
> +			tst_brk_(file, lineno, TBROK,
> +				"%s(): Attribute value too long", __func__);
> +			return -1;
> +		}
> +
> +		offset = NLMSG_ALIGN(ctx->datalen);
> +		ret = tst_rtnl_add_attr(file, lineno, ctx, list[i].type,
> +			list[i].data, list[i].len);
> +
> +		if (!ret)
> +			return -1;
> +
> +		if (list[i].sublist) {
> +			struct rtattr *attr;
> +
> +			ret = tst_rtnl_add_attr_list(file, lineno, ctx,
> +				list[i].sublist);
> +
> +			if (ret < 0)
> +				return ret;
> +
> +			attr = (struct rtattr *)(ctx->buffer + offset);
> +
> +			if (ctx->datalen - offset > USHRT_MAX) {
> +				tst_brk_(file, lineno, TBROK,
> +					"%s(): Sublist too long", __func__);
> +				return -1;
> +			}
> +
> +			attr->rta_len = ctx->datalen - offset;
> +		}
> +	}
> +
> +	return i;
> +}
> +
> +int tst_rtnl_check_acks(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, struct tst_rtnl_message *res)
> +{
> +	struct nlmsghdr *msg = (struct nlmsghdr *)ctx->buffer;
> +	int size_left = ctx->datalen;
> +
> +	for (; size_left > 0 && NLMSG_OK(msg, size_left);
> +		msg = NLMSG_NEXT(msg, size_left)) {
> +
> +		if (!(msg->nlmsg_flags & NLM_F_ACK))
> +			continue;
> +
> +		while (res->header && res->header->nlmsg_seq != msg->nlmsg_seq)
> +			res++;
> +
> +		if (!res->err || res->header->nlmsg_seq != msg->nlmsg_seq) {
> +			tst_brk_(file, lineno, TBROK,
> +				"No ACK found for Netlink message %u",
> +				msg->nlmsg_seq);
> +			return 0;
> +		}
> +
> +		if (res->err->error) {
> +			TST_ERR = -res->err->error;
> +			return 0;
> +		}
> +	}
> +
> +	return 1;
> +}
> +
> +int tst_rtnl_send_validate(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)
> +{
> +	struct tst_rtnl_message *response;
> +	int ret;
> +
> +	TST_ERR = 0;
> +
> +	if (tst_rtnl_send(file, lineno, ctx) <= 0)
> +		return 0;
> +
> +	tst_rtnl_wait(ctx);
> +	response = tst_rtnl_recv(file, lineno, ctx);
> +
> +	if (!response)
> +		return 0;
> +
> +	ret = tst_rtnl_check_acks(file, lineno, ctx, response);
> +	tst_rtnl_free_message(response);
> +	return ret;
> +}
> -- 
> 2.31.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library
  2021-04-27 13:41   ` Cyril Hrubis
@ 2021-04-27 14:14     ` Martin Doucha
  2021-04-27 14:35       ` Cyril Hrubis
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Doucha @ 2021-04-27 14:14 UTC (permalink / raw)
  To: ltp

On 27. 04. 21 15:41, Cyril Hrubis wrote:
> Hi!
>> +static int tst_rtnl_grow_buffer(const char *file, const int lineno,
>> +	struct tst_rtnl_context *ctx, size_t size)
>> +{
>> +	size_t needed, offset, curlen = NLMSG_ALIGN(ctx->datalen);
>> +	char *buf;
>> +
>> +	if (ctx->bufsize - curlen >= size)
>> +		return 1;
>> +
>> +	needed = size - (ctx->bufsize - curlen);
>> +	size = ctx->bufsize + (ctx->bufsize > needed ? ctx->bufsize : needed);
>> +	size = NLMSG_ALIGN(size);
>> +	buf = safe_realloc(file, lineno, ctx->buffer, size);
>> +
>> +	if (!buf)
>> +		return 0;
> 
> You are calling safe_realloc() here yet you check the return value. And
> it's the same for safe_malloc(), safe_bind(), safe_socket() and a few
> more in the code.
> 
> So either we get rid of the error checks and of the error
> propagation or we avoid using safe_ variants.

Both rtnetlink and netdevice management functions will be called in
cleanup() so I have to assume that the safe functions will only print a
warning instead of terminating the program. But I still want to use the
error reporting code in the safe functions.

> Other than that the code looks sane but it's hard to review the API
> without an example that would excersize it. What about adding something
> simple in newlib_tests?

There are short examples for both rtnetlink and netdevice management in
the cover letter. The netdevice library itself is also a detailed
example for the rtnetlink API. The final patchset will include the
SADDNS CVE test which will use most of the netdevice management
functions in setup().

>> +void tst_rtnl_free_context(const char *file, const int lineno,
>> +	struct tst_rtnl_context *ctx)
> 
> This should be probably called destroy_context() but that is very minor.

OK, why not.

>> +int tst_rtnl_wait(struct tst_rtnl_context *ctx)
>> +{
>> +	fd_set fdlist;
>> +	struct timeval timeout = {0};
>> +
>> +	FD_ZERO(&fdlist);
>> +	FD_SET(ctx->socket, &fdlist);
>> +	timeout.tv_sec = 1;
>> +
>> +	return select(ctx->socket + 1, &fdlist, NULL, NULL, &timeout);
> 
> I find the poll() syscall to have a bit saner API than this.

I'll have a look at it.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library
  2021-04-27 14:14     ` Martin Doucha
@ 2021-04-27 14:35       ` Cyril Hrubis
  0 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2021-04-27 14:35 UTC (permalink / raw)
  To: ltp

Hi!
> > You are calling safe_realloc() here yet you check the return value. And
> > it's the same for safe_malloc(), safe_bind(), safe_socket() and a few
> > more in the code.
> > 
> > So either we get rid of the error checks and of the error
> > propagation or we avoid using safe_ variants.
> 
> Both rtnetlink and netdevice management functions will be called in
> cleanup() so I have to assume that the safe functions will only print a
> warning instead of terminating the program. But I still want to use the
> error reporting code in the safe functions.

Right, I've realized that too, we have to return to the caller in that
case to carry on with the cleanup.

> > Other than that the code looks sane but it's hard to review the API
> > without an example that would excersize it. What about adding something
> > simple in newlib_tests?
> 
> There are short examples for both rtnetlink and netdevice management in
> the cover letter. The netdevice library itself is also a detailed
> example for the rtnetlink API. The final patchset will include the
> SADDNS CVE test which will use most of the netdevice management
> functions in setup().

Ah I've missed that. We should add it somewhere to the doc/ directory
since the cover letter would end up burried in the mailing list history.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library
  2021-04-26 11:19 ` [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library Martin Doucha
  2021-04-27 13:41   ` Cyril Hrubis
@ 2021-04-27 15:44   ` Cyril Hrubis
  1 sibling, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2021-04-27 15:44 UTC (permalink / raw)
  To: ltp

Hi!
> This library provides simple interface for creating arbitrary rtnetlink
> messages with complex attributes, sending requests and receiving results.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  include/tst_rtnetlink.h | 105 +++++++++++
>  lib/tst_rtnetlink.c     | 399 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 504 insertions(+)
>  create mode 100644 include/tst_rtnetlink.h
>  create mode 100644 lib/tst_rtnetlink.c
> 
> diff --git a/include/tst_rtnetlink.h b/include/tst_rtnetlink.h
> new file mode 100644
> index 000000000..3b307fbec
> --- /dev/null
> +++ b/include/tst_rtnetlink.h
> @@ -0,0 +1,105 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (c) 2021 Linux Test Project
> + */
> +
> +#ifndef TST_RTNETLINK_H
> +#define TST_RTNETLINK_H
> +
> +struct tst_rtnl_context;
> +
> +struct tst_rtnl_attr_list {
> +	unsigned short type;
> +	const void *data;
> +	ssize_t len;
> +	const struct tst_rtnl_attr_list *sublist;
> +};
> +
> +struct tst_rtnl_message {
> +	struct nlmsghdr *header;
> +	struct nlmsgerr *err;
> +	void *payload;
> +	size_t payload_size;
> +};
> +
> +/* Open a netlink socket */
> +struct tst_rtnl_context *tst_rtnl_create_context(const char *file,
> +	const int lineno);
> +#define RTNL_CREATE_CONTEXT() tst_rtnl_create_context(__FILE__, __LINE__)
> +
> +/* Free a tst_rtnl_message array returned by tst_rtnl_recv() */
> +void tst_rtnl_free_message(struct tst_rtnl_message *msg);
> +#define RTNL_FREE_MESSAGE tst_rtnl_free_message
> +
> +/* Close netlink socket */
> +void tst_rtnl_free_context(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx);
> +#define RTNL_FREE_CONTEXT(ctx) tst_rtnl_free_context(__FILE__, __LINE__, (ctx))
> +
> +/* Send all messages in given buffer */
> +int tst_rtnl_send(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx);
> +#define RTNL_SEND(ctx) tst_rtnl_send(__FILE__, __LINE__, (ctx))
> +
> +/* Send all messages in given buffer and validate kernel response */
> +int tst_rtnl_send_validate(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx);
> +#define RTNL_SEND_VALIDATE(ctx) \
> +	tst_rtnl_send_validate(__FILE__, __LINE__, (ctx))
> +
> +/* Wait until data is available for reading from the netlink socket */
> +int tst_rtnl_wait(struct tst_rtnl_context *ctx);
> +#define RTNL_WAIT tst_rtnl_wait
> +
> +/*
> + * Read from netlink socket and return an array of partially parsed messages.
> + * header == NULL indicates end of array.
> + */
> +struct tst_rtnl_message *tst_rtnl_recv(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx);
> +#define RTNL_RECV(ctx) tst_rtnl_recv(__FILE__, __LINE__, (ctx))
> +
> +/* Add new message to buffer */
> +int tst_rtnl_add_message(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, const struct nlmsghdr *header,
> +	const void *payload, size_t payload_size);
> +#define RTNL_ADD_MESSAGE(ctx, header, payload, psize) \
> +	tst_rtnl_add_message(__FILE__, __LINE__, (ctx), (header), (payload), \
> +		(psize))
> +
> +/* Add arbitrary attribute to last message */
> +int tst_rtnl_add_attr(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, unsigned short type, const void *data,
> +	unsigned short len);
> +#define RTNL_ADD_ATTR(ctx, type, data, len) \
> +	tst_rtnl_add_attr(__FILE__, __LINE__, (ctx), (type), (data), (len))
> +
> +/* Add string attribute to last message */
> +int tst_rtnl_add_attr_string(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, unsigned short type, const char *data);
> +#define RTNL_ADD_ATTR_STRING(ctx, type, data) \
> +	tst_rtnl_add_attr_string(__FILE__, __LINE__, (ctx), (type), (data))
> +
> +/*
> + * Add list of arbitrary attributes to last message. The list is terminated
> + * by attribute with negative length. Nested sublists are supported.
> + */
> +int tst_rtnl_add_attr_list(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, const struct tst_rtnl_attr_list *list);
> +#define RTNL_ADD_ATTR_LIST(ctx, list) \
> +	tst_rtnl_add_attr_list(__FILE__, __LINE__, (ctx), (list))
> +
> +/* Check that all sent messages with NLM_F_ACK flag have been acked without
> + * error. Usage:
> + *
> + * tst_rtnl_send(ctx);
> + * tst_rtnl_wait(ctx);
> + * response = tst_rtnl_recv(ctx);
> + * if (!tst_rtnl_check_acks(ctx, response)) { ... }
> + * tst_rtnl_free_message(response);
> + */
> +int tst_rtnl_check_acks(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, struct tst_rtnl_message *response);
> +#define RTNL_CHECK_ACKS(ctx, response) \
> +	tst_rtnl_context(__FILE__, __LINE__, (ctx), (response))
> +
> +#endif /* TST_RTNETLINK_H */
> diff --git a/lib/tst_rtnetlink.c b/lib/tst_rtnetlink.c
> new file mode 100644
> index 000000000..bb6116d57
> --- /dev/null
> +++ b/lib/tst_rtnetlink.c
> @@ -0,0 +1,399 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Linux Test Project
> + */
> +
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <asm/types.h>
> +#include <linux/netlink.h>
> +#include <linux/rtnetlink.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/select.h>
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "tst_rtnetlink.h"
> +
> +struct tst_rtnl_context {
> +	int socket;
> +	pid_t pid;
> +	uint32_t seq;
> +	size_t bufsize, datalen;
> +	char *buffer;
> +	struct nlmsghdr *curmsg;
> +};
> +
> +static int tst_rtnl_grow_buffer(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, size_t size)
> +{
> +	size_t needed, offset, curlen = NLMSG_ALIGN(ctx->datalen);
> +	char *buf;
> +
> +	if (ctx->bufsize - curlen >= size)
> +		return 1;
> +
> +	needed = size - (ctx->bufsize - curlen);
> +	size = ctx->bufsize + (ctx->bufsize > needed ? ctx->bufsize : needed);
> +	size = NLMSG_ALIGN(size);
> +	buf = safe_realloc(file, lineno, ctx->buffer, size);
> +
> +	if (!buf)
> +		return 0;
> +
> +	memset(buf + ctx->bufsize, 0, size - ctx->bufsize);
> +	offset = ((char *)ctx->curmsg) - ctx->buffer;
> +	ctx->buffer = buf;
> +	ctx->curmsg = (struct nlmsghdr *)(buf + offset);
> +	ctx->bufsize = size;
> +	return 1;
> +}
> +
> +struct tst_rtnl_context *tst_rtnl_create_context(const char *file,
> +	const int lineno)
> +{
> +	struct tst_rtnl_context *ctx;
> +	struct sockaddr_nl addr = {0};
> +
> +	ctx = safe_malloc(file, lineno, NULL, sizeof(struct tst_rtnl_context));
> +
> +	if (!ctx)
> +		return NULL;
> +
> +	ctx->pid = 0;
> +	ctx->seq = 0;
> +	ctx->bufsize = 1024;
> +	ctx->datalen = 0;
> +	ctx->curmsg = NULL;
> +	ctx->socket = safe_socket(file, lineno, NULL, AF_NETLINK,
> +		SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_ROUTE);
> +	addr.nl_family = AF_NETLINK;
> +
> +	if (ctx->socket < 0) {
> +		free(ctx);
> +		return NULL;
> +	}
> +
> +	if (safe_bind(file, lineno, NULL, ctx->socket, (struct sockaddr *)&addr,
> +		sizeof(addr))) {

safe_close() is missing here.

Also why don't we use the kernel style goto error handling here, that
would be probably easier to write. I.e.

	if (ctx->socket < 0)
		goto err0;

	if (safe_bind(...))
		goto err1;

	...

	return ctx;
err1:
	safe_close(file, lineno, NULL, ctx->socket);
err0:
	free(ctx);
	return NULL;

> +		free(ctx);
> +		return NULL;
> +	}
> +
> +	ctx->buffer = safe_malloc(file, lineno, NULL, ctx->bufsize);
> +
> +	if (!ctx->buffer) {
> +		safe_close(file, lineno, NULL, ctx->socket);
> +		free(ctx);
> +		return NULL;
> +	}
> +
> +	memset(ctx->buffer, 0, ctx->bufsize);
> +	return ctx;
> +}
> +
> +void tst_rtnl_free_message(struct tst_rtnl_message *msg)
> +{
> +	if (!msg)
> +		return;
> +
> +	// all ptr->header and ptr->info pointers point to the same buffer
> +	// msg->header is the start of the buffer
> +	free(msg->header);
> +	free(msg);
> +}
> +
> +void tst_rtnl_free_context(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)
> +{
> +	safe_close(file, lineno, NULL, ctx->socket);
> +	free(ctx->buffer);
> +	free(ctx);
> +}
> +
> +int tst_rtnl_send(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)
> +{
> +	struct sockaddr_nl addr = {0};
> +	struct iovec iov;
> +	struct msghdr msg = {0};
> +	int ret;
> +
> +	if (!ctx->curmsg) {
> +		tst_brk_(file, lineno, TBROK, "%s(): No message to send",
> +			__func__);
> +		return 0;
> +	}
> +
> +	if (ctx->curmsg->nlmsg_flags & NLM_F_MULTI) {
> +		size_t size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
> +
> +		if (!tst_rtnl_grow_buffer(file, lineno, ctx, NLMSG_SPACE(0)))
> +			return 0;
> +
> +		ctx->curmsg = NLMSG_NEXT(ctx->curmsg, size);
> +		memset(ctx->curmsg, 0, sizeof(struct nlmsghdr));
> +		ctx->curmsg->nlmsg_len = NLMSG_LENGTH(0);
> +		ctx->curmsg->nlmsg_type = NLMSG_DONE;
> +		ctx->curmsg->nlmsg_flags = 0;
> +		ctx->curmsg->nlmsg_seq = ctx->seq++;
> +		ctx->curmsg->nlmsg_pid = ctx->pid;
> +		ctx->datalen = NLMSG_ALIGN(ctx->datalen) + NLMSG_LENGTH(0);

If we add one if (lenth) before the memcpy() we can just call
tst_rtnl_add_message(file, lineno, ctx, &header, NULL, 0) here instead.

> +	}
> +	addr.nl_family = AF_NETLINK;
> +	iov.iov_base = ctx->buffer;
> +	iov.iov_len = ctx->datalen;
> +	msg.msg_name = &addr;
> +	msg.msg_namelen = sizeof(addr);
> +	msg.msg_iov = &iov;
> +	msg.msg_iovlen = 1;
> +
> +	ret = safe_sendmsg(file, lineno, ctx->datalen, ctx->socket, &msg, 0);
> +
> +	if (ret > 0)
> +		ctx->curmsg = NULL;
> +
> +	return ret;
> +}
> +
> +int tst_rtnl_wait(struct tst_rtnl_context *ctx)
> +{
> +	fd_set fdlist;
> +	struct timeval timeout = {0};
> +
> +	FD_ZERO(&fdlist);
> +	FD_SET(ctx->socket, &fdlist);
> +	timeout.tv_sec = 1;
> +
> +	return select(ctx->socket + 1, &fdlist, NULL, NULL, &timeout);
> +}
> +
> +struct tst_rtnl_message *tst_rtnl_recv(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)
> +{
> +	char *buffer, tmp;
> +	struct tst_rtnl_message *ret;
> +	struct nlmsghdr *ptr;
> +	ssize_t size;
> +	int i, size_left, msgcount;
> +
> +	errno = 0;
> +	size = recv(ctx->socket, &tmp, 1, MSG_DONTWAIT | MSG_PEEK | MSG_TRUNC);
> +
> +	if (size <= 0) {
> +		if (errno != EAGAIN)
> +			tst_brk_(file, lineno, TBROK | TERRNO, "recv() failed");
> +		return NULL;
> +	}
> +
> +	buffer = safe_malloc(file, lineno, NULL, size);
> +
> +	if (!buffer)
> +		return NULL;
> +
> +	size = safe_recv(file, lineno, size, ctx->socket, buffer, size, 0);
> +
> +	if (size <= 0) {
> +		free(buffer);
> +		return NULL;
> +	}
> +
> +	ptr = (struct nlmsghdr *)buffer;
> +	size_left = size;
> +	msgcount = 0;
> +
> +	for (; size_left > 0 && NLMSG_OK(ptr, size_left); msgcount++)
> +		ptr = NLMSG_NEXT(ptr, size_left);
> +
> +	ret = safe_malloc(file, lineno, NULL,
> +		(msgcount + 1) * sizeof(struct tst_rtnl_message));
                     ^
		     Maybe store this size to a variable so that we
		     don't have to repeat it for the memset()?
> +
> +	if (!ret) {
> +		free(buffer);
> +		return NULL;
> +	}
> +
> +	memset(ret, 0, (msgcount + 1) * sizeof(struct tst_rtnl_message));
> +	ptr = (struct nlmsghdr *)buffer;
> +	size_left = size;
> +
> +	for (i = 0; i < msgcount; i++, ptr = NLMSG_NEXT(ptr, size_left)) {
> +		ret[i].header = ptr;
> +		ret[i].payload = NLMSG_DATA(ptr);
> +		ret[i].payload_size = NLMSG_PAYLOAD(ptr, 0);
> +
> +		if (ptr->nlmsg_type == NLMSG_ERROR)
> +			ret[i].err = NLMSG_DATA(ptr);
> +	}
> +
> +	return ret;
> +}
> +
> +int tst_rtnl_add_message(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, const struct nlmsghdr *header,
> +	const void *payload, size_t payload_size)
> +{
> +	size_t size;
> +	unsigned int extra_flags = 0;
> +
> +	if (!tst_rtnl_grow_buffer(file, lineno, ctx, NLMSG_SPACE(payload_size)))
> +		return 0;
> +
> +	if (!ctx->curmsg) {
> +		/*
> +		 * datalen may hold the size of last sent message for ACK
> +		 * checking, reset it back to 0 here
> +		 */
> +		ctx->datalen = 0;
> +		ctx->curmsg = (struct nlmsghdr *)ctx->buffer;
> +	} else {
> +		size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
> +
> +		extra_flags = NLM_F_MULTI;
> +		ctx->curmsg->nlmsg_flags |= extra_flags;
> +		ctx->curmsg = NLMSG_NEXT(ctx->curmsg, size);
> +		ctx->datalen = NLMSG_ALIGN(ctx->datalen);
> +	}
> +
> +	*ctx->curmsg = *header;
> +	ctx->curmsg->nlmsg_len = NLMSG_LENGTH(payload_size);
> +	ctx->curmsg->nlmsg_flags |= extra_flags;
> +	ctx->curmsg->nlmsg_seq = ctx->seq++;
> +	ctx->curmsg->nlmsg_pid = ctx->pid;
> +	memcpy(NLMSG_DATA(ctx->curmsg), payload, payload_size);
> +	ctx->datalen += ctx->curmsg->nlmsg_len;
> +	return 1;
> +}
> +
> +int tst_rtnl_add_attr(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, unsigned short type,
> +	const void *data, unsigned short len)
> +{
> +	size_t size;
> +	struct rtattr *attr;
> +
> +	if (!ctx->curmsg) {
> +		tst_brk_(file, lineno, TBROK,
> +			"%s(): No message to add attributes to", __func__);
> +		return 0;
> +	}
> +
> +	if (!tst_rtnl_grow_buffer(file, lineno, ctx, RTA_SPACE(len)))
> +		return 0;
> +
> +	size = NLMSG_ALIGN(ctx->curmsg->nlmsg_len);
> +	attr = (struct rtattr *)(((char *)ctx->curmsg) + size);
> +	attr->rta_type = type;
> +	attr->rta_len = RTA_LENGTH(len);
> +	memcpy(RTA_DATA(attr), data, len);
> +	ctx->curmsg->nlmsg_len = size + attr->rta_len;
> +	ctx->datalen = NLMSG_ALIGN(ctx->datalen) + attr->rta_len;
> +	return 1;
> +}
> +
> +int tst_rtnl_add_attr_string(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, unsigned short type,
> +	const char *data)
> +{
> +	return tst_rtnl_add_attr(file, lineno, ctx, type, data,
> +		strlen(data) + 1);
> +}
> +
> +int tst_rtnl_add_attr_list(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx,
> +	const struct tst_rtnl_attr_list *list)
> +{
> +	int i, ret;
> +	size_t offset;
> +
> +	for (i = 0; list[i].len >= 0; i++) {
> +		if (list[i].len > USHRT_MAX) {
> +			tst_brk_(file, lineno, TBROK,
> +				"%s(): Attribute value too long", __func__);
> +			return -1;
> +		}
> +
> +		offset = NLMSG_ALIGN(ctx->datalen);
> +		ret = tst_rtnl_add_attr(file, lineno, ctx, list[i].type,
> +			list[i].data, list[i].len);
> +
> +		if (!ret)
> +			return -1;
> +
> +		if (list[i].sublist) {
> +			struct rtattr *attr;
> +
> +			ret = tst_rtnl_add_attr_list(file, lineno, ctx,
> +				list[i].sublist);
> +
> +			if (ret < 0)
> +				return ret;
> +
> +			attr = (struct rtattr *)(ctx->buffer + offset);
> +
> +			if (ctx->datalen - offset > USHRT_MAX) {
> +				tst_brk_(file, lineno, TBROK,
> +					"%s(): Sublist too long", __func__);
> +				return -1;
> +			}
> +
> +			attr->rta_len = ctx->datalen - offset;
> +		}
> +	}
> +
> +	return i;
> +}
> +
> +int tst_rtnl_check_acks(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx, struct tst_rtnl_message *res)
> +{
> +	struct nlmsghdr *msg = (struct nlmsghdr *)ctx->buffer;
> +	int size_left = ctx->datalen;
> +
> +	for (; size_left > 0 && NLMSG_OK(msg, size_left);
> +		msg = NLMSG_NEXT(msg, size_left)) {
> +
> +		if (!(msg->nlmsg_flags & NLM_F_ACK))
> +			continue;
> +
> +		while (res->header && res->header->nlmsg_seq != msg->nlmsg_seq)
> +			res++;
> +
> +		if (!res->err || res->header->nlmsg_seq != msg->nlmsg_seq) {
> +			tst_brk_(file, lineno, TBROK,
> +				"No ACK found for Netlink message %u",
> +				msg->nlmsg_seq);
> +			return 0;
> +		}
> +
> +		if (res->err->error) {
> +			TST_ERR = -res->err->error;
> +			return 0;
> +		}
> +	}
> +
> +	return 1;
> +}
> +
> +int tst_rtnl_send_validate(const char *file, const int lineno,
> +	struct tst_rtnl_context *ctx)
> +{
> +	struct tst_rtnl_message *response;
> +	int ret;
> +
> +	TST_ERR = 0;
> +
> +	if (tst_rtnl_send(file, lineno, ctx) <= 0)
> +		return 0;
> +
> +	tst_rtnl_wait(ctx);
> +	response = tst_rtnl_recv(file, lineno, ctx);
> +
> +	if (!response)
> +		return 0;
> +
> +	ret = tst_rtnl_check_acks(file, lineno, ctx, response);
> +	tst_rtnl_free_message(response);
> +	return ret;
> +}

I had a closer look at the library and minus a few minor things it looks
good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces
  2021-04-26 11:19 ` [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces Martin Doucha
@ 2021-04-28 10:27   ` Cyril Hrubis
  2021-04-28 13:42     ` Martin Doucha
  0 siblings, 1 reply; 14+ messages in thread
From: Cyril Hrubis @ 2021-04-28 10:27 UTC (permalink / raw)
  To: ltp

Hi!
> The library currently supports:
> - creating a virtual ethernet device pair
> - removing network interfaces
> - enabling or disabling a network interface
> - managing interface addresses
> - managing routing table entries
> - moving network interfaces between network namespaces
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  include/tst_netdevice.h | 120 ++++++++++
>  lib/tst_netdevice.c     | 506 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 626 insertions(+)
>  create mode 100644 include/tst_netdevice.h
>  create mode 100644 lib/tst_netdevice.c
> 
> diff --git a/include/tst_netdevice.h b/include/tst_netdevice.h
> new file mode 100644
> index 000000000..69f559fdd
> --- /dev/null
> +++ b/include/tst_netdevice.h
> @@ -0,0 +1,120 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (c) 2021 Linux Test Project
> + */
> +
> +#ifndef TST_NETDEVICE_H
> +#define TST_NETDEVICE_H
> +
> +/* Find device index for given network interface name. */
> +int tst_netdevice_index(const char *file, const int lineno, const char *ifname);
> +#define NETDEVICE_INDEX(ifname) \
> +	tst_netdevice_index(__FILE__, __LINE__, (ifname))
> +
> +/* Activate or deactivate network interface */
> +int tst_netdevice_activate(const char *file, const int lineno,
> +	const char *ifname, int up);
> +#define NETDEVICE_ACTIVATE(ifname, up) \
> +	tst_netdevice_activate(__FILE__, __LINE__, (ifname), (up))
> +
> +/* Create a connected pair of virtual network devices */
> +int tst_create_veth_pair(const char *file, const int lineno,
> +	const char *ifname1, const char *ifname2);
> +#define CREATE_VETH_PAIR(ifname1, ifname2) \
> +	tst_create_veth_pair(__FILE__, __LINE__, (ifname1), (ifname2))
> +
> +int tst_remove_netdevice(const char *file, const int lineno,
> +	const char *ifname);
> +#define REMOVE_NETDEVICE(ifname) \
> +	tst_remove_netdevice(__FILE__, __LINE__, (ifname))
> +
> +int tst_netdevice_add_address(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *address,
> +	unsigned int prefix, size_t addrlen, unsigned int flags);
> +#define NETDEVICE_ADD_ADDRESS(ifname, family, address, prefix, addrlen, flags) \
> +	tst_netdevice_add_address(__FILE__, __LINE__, (ifname), (family), \
> +		(address), (prefix), (addrlen), (flags))
> +
> +int tst_netdevice_add_address_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t address, unsigned int prefix,
> +	unsigned int flags);
> +#define NETDEVICE_ADD_ADDRESS_INET(ifname, address, prefix, flags) \
> +	tst_netdevice_add_address_inet(__FILE__, __LINE__, (ifname), \
> +		(address), (prefix), (flags))
> +
> +int tst_netdevice_remove_address(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *address,
> +	size_t addrlen);
> +#define NETDEVICE_REMOVE_ADDRESS(ifname, family, address, addrlen) \
> +	tst_netdevice_remove_address(__FILE__, __LINE__, (ifname), (family), \
> +		(address), (addrlen))
> +
> +int tst_netdevice_remove_address_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t address);
> +#define NETDEVICE_REMOVE_ADDRESS_INET(ifname, address) \
> +	tst_netdevice_remove_address_inet(__FILE__, __LINE__, (ifname), \
> +		(address))
> +
> +int tst_netdevice_change_ns_fd(const char *file, const int lineno,
> +	const char *ifname, int nsfd);
> +#define NETDEVICE_CHANGE_NS_FD(ifname, nsfd) \
> +	tst_netdevice_change_ns_fd(__FILE__, __LINE__, (ifname), (nsfd))
> +
> +int tst_netdevice_change_ns_pid(const char *file, const int lineno,
> +	const char *ifname, pid_t nspid);
> +#define NETDEVICE_CHANGE_NS_PID(ifname, nspid) \
> +	tst_netdevice_change_ns_pid(__FILE__, __LINE__, (ifname), (nspid))
> +
> +/*
> + * Add new static entry to main routing table. If you specify gateway address,
> + * the interface name is optional.
> + */
> +int tst_netdevice_add_route(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *srcaddr,
> +	unsigned int srcprefix, size_t srclen, const void *dstaddr,
> +	unsigned int dstprefix, size_t dstlen, const void *gateway,
> +	size_t gatewaylen);
> +#define NETDEVICE_ADD_ROUTE(ifname, family, srcaddr, srcprefix, srclen, \
> +	dstaddr, dstprefix, dstlen, gateway, gatewaylen) \
> +	tst_netdevice_add_route(__FILE__, __LINE__, (ifname), (family), \
> +		(srcaddr), (srcprefix), (srclen), (dstaddr), (dstprefix), \
> +		(dstlen), (gateway), (gatewaylen))
> +
> +/*
> + * Simplified function for adding IPv4 static route. If you set srcprefix
> + * or dstprefix to 0, the corresponding address will be ignored. Interface
> + * name is optional if gateway address is non-zero.
> + */
> +int tst_netdevice_add_route_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
> +	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway);
> +#define NETDEVICE_ADD_ROUTE_INET(ifname, srcaddr, srcprefix, dstaddr, \
> +	dstprefix, gateway) \
> +	tst_netdevice_add_route_inet(__FILE__, __LINE__, (ifname), (srcaddr), \
> +		(srcprefix), (dstaddr), (dstprefix), (gateway))
> +
> +/*
> + * Remove static entry from main routing table.
> + */
> +int tst_netdevice_remove_route(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *srcaddr,
> +	unsigned int srcprefix, size_t srclen, const void *dstaddr,
> +	unsigned int dstprefix, size_t dstlen, const void *gateway,
> +	size_t gatewaylen);
> +#define NETDEVICE_REMOVE_ROUTE(ifname, family, srcaddr, srcprefix, srclen, \
> +	dstaddr, dstprefix, dstlen, gateway, gatewaylen) \
> +	tst_netdevice_remove_route(__FILE__, __LINE__, (ifname), (family), \
> +		(srcaddr), (srcprefix), (srclen), (dstaddr), (dstprefix), \
> +		(dstlen), (gateway), (gatewaylen))
> +
> +/*
> + * Simplified function for removing IPv4 static route.
> + */
> +int tst_netdevice_remove_route_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
> +	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway);
> +#define NETDEVICE_REMOVE_ROUTE_INET(ifname, srcaddr, srcprefix, dstaddr, \
> +	dstprefix, gateway) \
> +	tst_netdevice_remove_route_inet(__FILE__, __LINE__, (ifname), \
> +		(srcaddr), (srcprefix), (dstaddr), (dstprefix), (gateway))
> +
> +#endif /* TST_NETDEVICE_H */
> diff --git a/lib/tst_netdevice.c b/lib/tst_netdevice.c
> new file mode 100644
> index 000000000..541541b11
> --- /dev/null
> +++ b/lib/tst_netdevice.c
> @@ -0,0 +1,506 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Linux Test Project
> + */
> +
> +#include <asm/types.h>
> +#include <linux/netlink.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/veth.h>
> +#include <sys/socket.h>
> +#include <net/if.h>
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "tst_rtnetlink.h"
> +#include "tst_netdevice.h"
> +
> +static struct tst_rtnl_context *create_request(const char *file,
> +	const int lineno, unsigned int type, unsigned int flags,
> +	const void *payload, size_t psize)
> +{
> +	struct nlmsghdr header = {0};
> +	struct tst_rtnl_context *ctx;
> +
> +	ctx = tst_rtnl_create_context(file, lineno);
> +
> +	if (!ctx)
> +		return NULL;
> +
> +	header.nlmsg_type = type;
> +	header.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | flags;
> +
> +	if (!tst_rtnl_add_message(file, lineno, ctx, &header, payload, psize)) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return NULL;
> +	}
> +
> +	return ctx;
> +}
> +
> +int tst_netdevice_index(const char *file, const int lineno, const char *ifname)

The function name could be a bit more descriptive, what about
tst_netdev_idx_by_name() ?

> +{
> +	struct ifreq ifr;
> +	int sock;
> +
> +	if (strlen(ifname) >= IFNAMSIZ) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Network device name \"%s\" too long", ifname);
                                              ^
					      I usually use single
					      quotes inside C strings
					      but that is matter of
					      taste
> +		return -1;
> +	}
> +
> +	sock = safe_socket(file, lineno, NULL, AF_INET, SOCK_DGRAM, 0);
> +
> +	if (sock < 0)
> +		return -1;
> +
> +	strcpy(ifr.ifr_name, ifname);
> +	TEST(ioctl(sock, SIOCGIFINDEX, &ifr));
> +
> +	safe_close(file, lineno, NULL, sock);
> +
> +	if (TST_RET < 0) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"ioctl(SIOCGIFINDEX) failed");
> +	} else if (TST_RET) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Invalid ioctl(SIOCGIFINDEX) return value %ld",
> +			TST_RET);
> +	}

Using SAFE_IOCTL() would save us some lines here, but we would have to
add a few more variants to the safe macros header.

> +	return TST_RET ? -1 : ifr.ifr_ifindex;
> +}
> +
> +int tst_netdevice_activate(const char *file, const int lineno,
> +	const char *ifname, int up)

Naming this function activate is a bit confusing since it sets the
interface up or down depending on the up flag. What about
tst_netdev_state_set() ?

> +{
> +	struct ifreq ifr;
> +	int sock;
> +
> +	if (strlen(ifname) >= IFNAMSIZ) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Network device name \"%s\" too long", ifname);
> +		return -1;
> +	}
> +
> +	sock = safe_socket(file, lineno, NULL, AF_INET, SOCK_DGRAM, 0);
> +
> +	if (sock < 0)
> +		return -1;
> +
> +	strcpy(ifr.ifr_name, ifname);
> +	TEST(ioctl(sock, SIOCGIFFLAGS, &ifr));
> +
> +	if (TST_RET < 0) {
> +		safe_close(file, lineno, NULL, sock);
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"ioctl(SIOCGIFFLAGS) failed");
> +		return TST_RET;
> +	}
> +
> +	if (TST_RET) {
> +		safe_close(file, lineno, NULL, sock);
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Invalid ioctl(SIOCGIFFLAGS) return value %ld",
> +			TST_RET);
> +		return TST_RET;
> +	}
> +
> +	if (up)
> +		ifr.ifr_flags |= IFF_UP;
> +	else
> +		ifr.ifr_flags &= ~IFF_UP;
> +
> +	TEST(ioctl(sock, SIOCSIFFLAGS, &ifr));
> +	safe_close(file, lineno, NULL, sock);
> +
> +	if (TST_RET < 0) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"ioctl(SIOCSIFFLAGS) failed");
> +	} else if (TST_RET) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Invalid ioctl(SIOCSIFFLAGS) return value %ld",
> +			TST_RET);
> +	}
> +
> +	return TST_RET;
> +}
> +
> +int tst_create_veth_pair(const char *file, const int lineno,
> +	const char *ifname1, const char *ifname2)
> +{
> +	int ret;
> +	struct ifinfomsg info = {0};
> +	struct tst_rtnl_context *ctx;
> +	struct tst_rtnl_attr_list peerinfo[] = {
> +		{IFLA_IFNAME, ifname2, strlen(ifname2) + 1, NULL},
> +		{0, NULL, -1, NULL}
> +	};
> +	struct tst_rtnl_attr_list peerdata[] = {
> +		{VETH_INFO_PEER, &info, sizeof(info), peerinfo},
> +		{0, NULL, -1, NULL}
> +	};
> +	struct tst_rtnl_attr_list attrs[] = {
> +		{IFLA_IFNAME, ifname1, strlen(ifname1) + 1, NULL},
> +		{IFLA_LINKINFO, NULL, 0, (const struct tst_rtnl_attr_list[]){
> +			{IFLA_INFO_KIND, "veth", 4, NULL},
> +			{IFLA_INFO_DATA, NULL, 0, peerdata},
> +			{0, NULL, -1, NULL}
> +		}},
> +		{0, NULL, -1, NULL}
> +	};
> +
> +	if (strlen(ifname1) >= IFNAMSIZ) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Network device name \"%s\" too long", ifname1);
> +		return 0;
> +	}
> +
> +	if (strlen(ifname2) >= IFNAMSIZ) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Network device name \"%s\" too long", ifname2);
> +		return 0;
> +	}
> +
> +	info.ifi_family = AF_UNSPEC;

We can just initialize this inline like the rest, rigth?

> +	ctx = create_request(file, lineno, RTM_NEWLINK,
> +		NLM_F_CREATE | NLM_F_EXCL, &info, sizeof(info));
> +
> +	if (!ctx)
> +		return 0;
> +
> +	if (tst_rtnl_add_attr_list(file, lineno, ctx, attrs) != 2) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	ret = tst_rtnl_send_validate(file, lineno, ctx);
> +	tst_rtnl_free_context(file, lineno, ctx);
> +
> +	if (!ret) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Failed to create veth interfaces %s+%s", ifname1,
> +			ifname2);
> +	}
> +
> +	return ret;
> +}
> +
> +int tst_remove_netdevice(const char *file, const int lineno, const char *ifname)
> +{
> +	struct ifinfomsg info = {0};
> +	struct tst_rtnl_context *ctx;
> +	int ret;
> +
> +	if (strlen(ifname) >= IFNAMSIZ) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Network device name \"%s\" too long", ifname);
> +		return 0;
> +	}
> +
> +	info.ifi_family = AF_UNSPEC;

Here as well, inline initialization?

> +	ctx = create_request(file, lineno, RTM_DELLINK, 0, &info, sizeof(info));
> +
> +	if (!ctx)
> +		return 0;
> +
> +	if (!tst_rtnl_add_attr_string(file, lineno, ctx, IFLA_IFNAME, ifname)) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	ret = tst_rtnl_send_validate(file, lineno, ctx);
> +	tst_rtnl_free_context(file, lineno, ctx);
> +
> +	if (!ret) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Failed to remove netdevice %s", ifname);
> +	}
> +
> +	return ret;
> +}
> +
> +static int modify_address(const char *file, const int lineno,
> +	unsigned int action, unsigned int nl_flags, const char *ifname,
> +	unsigned int family, const void *address, unsigned int prefix,
> +	size_t addrlen, uint32_t addr_flags)
> +{
> +	struct ifaddrmsg info = {0};
> +	struct tst_rtnl_context *ctx;
> +	int index, ret;
> +
> +	index = tst_netdevice_index(file, lineno, ifname);
> +
> +	if (index < 0) {
> +		tst_brk_(file, lineno, TBROK, "Interface %s not found", ifname);
> +		return 0;
> +	}
> +
> +	info.ifa_family = family;
> +	info.ifa_prefixlen = prefix;
> +	info.ifa_index = index;

Here as well, inline initialization?

We can define the structure here and even include the index in the
initialization as well...

> +	ctx = create_request(file, lineno, action, nl_flags, &info,
> +		sizeof(info));
> +
> +	if (!ctx)
> +		return 0;
> +
> +
> +	if (!tst_rtnl_add_attr(file, lineno, ctx, IFA_FLAGS, &addr_flags,
> +		sizeof(uint32_t))) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	if (!tst_rtnl_add_attr(file, lineno, ctx, IFA_LOCAL, address,
> +		addrlen)) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	ret = tst_rtnl_send_validate(file, lineno, ctx);
> +	tst_rtnl_free_context(file, lineno, ctx);
> +
> +	if (!ret) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Failed to modify %s network address", ifname);
> +	}
> +
> +	return ret;
> +}
> +
> +int tst_netdevice_add_address(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *address,
> +	unsigned int prefix, size_t addrlen, unsigned int flags)
> +{
> +	return modify_address(file, lineno, RTM_NEWADDR,
> +		NLM_F_CREATE | NLM_F_EXCL, ifname, family, address, prefix,
> +		addrlen, flags);
> +}
> +
> +int tst_netdevice_add_address_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t address, unsigned int prefix,
> +	unsigned int flags)
> +{
> +	return tst_netdevice_add_address(file, lineno, ifname, AF_INET,
> +		&address, prefix, sizeof(address), flags);
> +}
> +
> +int tst_netdevice_remove_address(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *address,
> +	size_t addrlen)
> +{
> +	return modify_address(file, lineno, RTM_DELADDR, 0, ifname, family,
> +		address, 0, addrlen, 0);
> +}
> +
> +int tst_netdevice_remove_address_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t address)
> +{
> +	return tst_netdevice_remove_address(file, lineno, ifname, AF_INET,
> +		&address, sizeof(address));
> +}

I would consider adding the _inet one-liners as a static inline functions to
the header instead.

> +static int change_ns(const char *file, const int lineno, const char *ifname,
> +	unsigned short attr, uint32_t value)
> +{
> +	struct ifinfomsg info = {0};
> +	struct tst_rtnl_context *ctx;
> +	int ret;
> +
> +	if (strlen(ifname) >= IFNAMSIZ) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Network device name \"%s\" too long", ifname);
> +		return 0;
> +	}
> +
> +	info.ifi_family = AF_UNSPEC;
> +	ctx = create_request(file, lineno, RTM_NEWLINK, 0, &info, sizeof(info));
> +
> +	if (!tst_rtnl_add_attr_string(file, lineno, ctx, IFLA_IFNAME, ifname)) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	if (!tst_rtnl_add_attr(file, lineno, ctx, attr, &value,
> +		sizeof(uint32_t))) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	ret = tst_rtnl_send_validate(file, lineno, ctx);
> +	tst_rtnl_free_context(file, lineno, ctx);
> +
> +	if (!ret) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Failed to move %s to another namespace", ifname);
> +	}
> +
> +	return ret;
> +}
> +
> +int tst_netdevice_change_ns_fd(const char *file, const int lineno,
> +	const char *ifname, int nsfd)
> +{
> +	return change_ns(file, lineno, ifname, IFLA_NET_NS_FD, nsfd);
> +}
> +
> +int tst_netdevice_change_ns_pid(const char *file, const int lineno,
> +	const char *ifname, pid_t nspid)
> +{
> +	return change_ns(file, lineno, ifname, IFLA_NET_NS_PID, nspid);
> +}
> +
> +static int modify_route(const char *file, const int lineno, unsigned int action,
> +	unsigned int flags, const char *ifname, unsigned int family,
> +	const void *srcaddr, unsigned int srcprefix, size_t srclen,
> +	const void *dstaddr, unsigned int dstprefix, size_t dstlen,
> +	const void *gateway, size_t gatewaylen)
> +{
> +	struct rtmsg info = {0};
> +	struct tst_rtnl_context *ctx;
> +	int ret;
> +	int32_t index;
> +
> +	if (!ifname && !gateway) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Interface name or gateway address required");
> +		return 0;
> +	}
> +
> +	if (ifname && strlen(ifname) >= IFNAMSIZ) {
> +		tst_brk_(file, lineno, TBROK,
> +			"Network device name \"%s\" too long", ifname);
> +		return 0;
> +	}
> +
> +	if (ifname) {
> +		index = tst_netdevice_index(file, lineno, ifname);
> +
> +		if (index < 0)
> +			return 0;
> +	}
> +
> +	info.rtm_family = family;
> +	info.rtm_dst_len = dstprefix;
> +	info.rtm_src_len = srcprefix;
> +	info.rtm_table = RT_TABLE_MAIN;
> +	info.rtm_protocol = RTPROT_STATIC;
> +	info.rtm_type = RTN_UNICAST;

Inline initialization as well?

> +	if (action == RTM_DELROUTE) {
> +		tst_res_(file, lineno, TINFO, "DELROUTE");
> +		info.rtm_scope = RT_SCOPE_NOWHERE;
> +	} else {
> +		tst_res_(file, lineno, TINFO, "ADDROUTE");
> +		info.rtm_scope = RT_SCOPE_UNIVERSE;
> +	}
> +
> +	ctx = create_request(file, lineno, action, flags, &info, sizeof(info));
> +
> +	if (srcaddr && !tst_rtnl_add_attr(file, lineno, ctx, RTA_SRC, srcaddr,
> +		srclen)) {

Maybe it would be a bit more readable if we put the whole
tst_rtnl_add_attr() on a separate line as:

	if (srcaddr &&
	    !tst_rtnl_add_attr(file, lineno, ctx, RTA_SRC, srcaddr, srclen)) {


	}

> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	if (dstaddr && !tst_rtnl_add_attr(file, lineno, ctx, RTA_DST, dstaddr,
> +		dstlen)) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	if (gateway && !tst_rtnl_add_attr(file, lineno, ctx, RTA_GATEWAY,
> +		gateway, gatewaylen)) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	if (ifname && !tst_rtnl_add_attr(file, lineno, ctx, RTA_OIF, &index,
> +		sizeof(index))) {
> +		tst_rtnl_free_context(file, lineno, ctx);
> +		return 0;
> +	}
> +
> +	ret = tst_rtnl_send_validate(file, lineno, ctx);
> +	tst_rtnl_free_context(file, lineno, ctx);
> +
> +	if (!ret) {
> +		tst_brk_(file, lineno, TBROK | TTERRNO,
> +			"Failed to modify network route");
> +	}
> +
> +	return ret;
> +}
> +
> +int tst_netdevice_add_route(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *srcaddr,
> +	unsigned int srcprefix, size_t srclen, const void *dstaddr,
> +	unsigned int dstprefix, size_t dstlen, const void *gateway,
> +	size_t gatewaylen)
> +{
> +	return modify_route(file, lineno, RTM_NEWROUTE,
> +		NLM_F_CREATE | NLM_F_EXCL, ifname, family, srcaddr, srcprefix,
> +		srclen, dstaddr, dstprefix, dstlen, gateway, gatewaylen);
> +}
> +
> +int tst_netdevice_add_route_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
> +	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway)
> +{
> +	void *src = NULL, *dst = NULL, *gw = NULL;
> +	size_t srclen = 0, dstlen = 0, gwlen = 0;
> +
> +	if (srcprefix) {
> +		src = &srcaddr;
> +		srclen = sizeof(srcaddr);
> +	}
> +
> +	if (dstprefix) {
> +		dst = &dstaddr;
> +		dstlen = sizeof(dstaddr);
> +	}
> +
> +	if (gateway) {
> +		gw = &gateway;
> +		gwlen = sizeof(gateway);
> +	}
> +
> +	return tst_netdevice_add_route(file, lineno, ifname, AF_INET, src,
> +		srcprefix, srclen, dst, dstprefix, dstlen, gw, gwlen);
> +}
> +
> +int tst_netdevice_remove_route(const char *file, const int lineno,
> +	const char *ifname, unsigned int family, const void *srcaddr,
> +	unsigned int srcprefix, size_t srclen, const void *dstaddr,
> +	unsigned int dstprefix, size_t dstlen, const void *gateway,
> +	size_t gatewaylen)
> +{
> +	return modify_route(file, lineno, RTM_DELROUTE, 0, ifname, family,
> +		srcaddr, srcprefix, srclen, dstaddr, dstprefix, dstlen,
> +		gateway, gatewaylen);
> +}
> +
> +int tst_netdevice_remove_route_inet(const char *file, const int lineno,
> +	const char *ifname, in_addr_t srcaddr, unsigned int srcprefix,
> +	in_addr_t dstaddr, unsigned int dstprefix, in_addr_t gateway)
> +{
> +	void *src = NULL, *dst = NULL, *gw = NULL;
> +	size_t srclen = 0, dstlen = 0, gwlen = 0;
> +
> +	if (srcprefix) {
> +		src = &srcaddr;
> +		srclen = sizeof(srcaddr);
> +	}
> +
> +	if (dstprefix) {
> +		dst = &dstaddr;
> +		dstlen = sizeof(dstaddr);
> +	}
> +
> +	if (gateway) {
> +		gw = &gateway;
> +		gwlen = sizeof(gateway);
> +	}
> +
> +	return tst_netdevice_remove_route(file, lineno, ifname, AF_INET, src,
> +		srcprefix, srclen, dst, dstprefix, dstlen, gw, gwlen);
> +}

This is nearly the same as add_route, maybe it would be easier if we had
modify_route_inet() that would fill in the parameters...


Overall the code looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces
  2021-04-28 10:27   ` Cyril Hrubis
@ 2021-04-28 13:42     ` Martin Doucha
  2021-04-28 14:34       ` Cyril Hrubis
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Doucha @ 2021-04-28 13:42 UTC (permalink / raw)
  To: ltp

On 28. 04. 21 12:27, Cyril Hrubis wrote:
> Hi!
> ...

Thanks, I'll apply the fixes and suggestions before I resubmit. Do you
have any comments on the header files from API user's perspective?

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces
  2021-04-28 13:42     ` Martin Doucha
@ 2021-04-28 14:34       ` Cyril Hrubis
  0 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2021-04-28 14:34 UTC (permalink / raw)
  To: ltp

Hi!
> Thanks, I'll apply the fixes and suggestions before I resubmit. Do you
> have any comments on the header files from API user's perspective?

The API looks reasonable, apart from the minor name changes I've pointed
out and shortening netdevice to just netdev.

Actually having a second look it may make sense to put the netlink
into a separate library probably to libs/libltpnl/

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-04-28 14:34 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-26 11:19 [LTP] [PATCH 0/4] RTNetlink and network device management library Martin Doucha
2021-04-26 11:19 ` [LTP] [PATCH 1/4] Add SAFE_REALLOC() helper function to LTP library Martin Doucha
2021-04-27 13:04   ` Cyril Hrubis
2021-04-26 11:19 ` [LTP] [PATCH 2/4] Add SAFE_RECV() " Martin Doucha
2021-04-27 13:06   ` Cyril Hrubis
2021-04-26 11:19 ` [LTP] [PATCH 3/4] RFC: Add rtnetlink helper library Martin Doucha
2021-04-27 13:41   ` Cyril Hrubis
2021-04-27 14:14     ` Martin Doucha
2021-04-27 14:35       ` Cyril Hrubis
2021-04-27 15:44   ` Cyril Hrubis
2021-04-26 11:19 ` [LTP] [PATCH 4/4] RFC: Add helper functions for managing network interfaces Martin Doucha
2021-04-28 10:27   ` Cyril Hrubis
2021-04-28 13:42     ` Martin Doucha
2021-04-28 14:34       ` Cyril Hrubis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.