Re: [PATH nft v2 01/18] mnl: fix error handling in mnl_batch_talk

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

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

On Mon, Aug 21, 2017 at 09:01:55PM +0200, Eric Leblond wrote:
> Hi,
> 
> On Mon, 2017-08-21 at 10:10 +0200, Pablo Neira Ayuso wrote:
> > Hi Eric,
> > 
> > On Sat, Aug 19, 2017 at 05:24:03PM +0200, Eric Leblond wrote:
> > > If one of the command is failing we should return an error.
> > 
> > Is this fixing up a real issue or it is something you need in a
> > follow
> > up patch?
> 
> Not really for current users of the function. But I think but it is an
> issue as the result of the function is success even when it fails.

Yes, this function should consistently return an error.

I'm attaching an amended patch, I think we can remove the explicit:

        ret = -1;

from nft_netlink().

I'll be applying what I'm attaching unless there is any concern.

[-- Attachment #2: 0001-mnl-fix-error-handling-in-mnl_batch_talk.patch --]
[-- Type: text/x-diff, Size: 2075 bytes --]

>From 206fdb25b7b53c164700a8cd7d7e659e058ad881 Mon Sep 17 00:00:00 2001
From: Eric Leblond <eric@regit.org>
Date: Thu, 24 Aug 2017 17:07:37 +0200
Subject: [PATCH] mnl: fix error handling in mnl_batch_talk

If one of the command is failing we should return an error.

Pablo says: "This is not a real issue since nft_netlink() returns an
error in case the list of errors is not empty. But we can indeed
simplify things by removing that explicit assignment in nft_netlink() so
mnl_batch_talk() consistently reports when if an error has happened.

Signee-off-by: Eric Leblond <eric@regit.org>
---
 src/main.c | 1 -
 src/mnl.c  | 7 +++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/main.c b/src/main.c
index 3519377b6e2c..c09d9f341b69 100644
--- a/src/main.c
+++ b/src/main.c
@@ -220,7 +220,6 @@ static int nft_netlink(struct nft_ctx *nft,
 				netlink_io_error(&ctx, &cmd->location,
 						 "Could not process rule: %s",
 						 strerror(err->err));
-				ret = -1;
 				errno = err->err;
 				if (err->seqnum == cmd->seqnum) {
 					mnl_err_list_free(err);
diff --git a/src/mnl.c b/src/mnl.c
index a770dc567d9f..69e24071b8f1 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -249,6 +249,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
 		.tv_sec		= 0,
 		.tv_usec	= 0
 	};
+	int err = 0;
 
 	ret = mnl_nft_socket_sendmsg(ctx);
 	if (ret == -1)
@@ -271,8 +272,10 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
 
 		ret = mnl_cb_run(rcv_buf, ret, 0, portid, &netlink_echo_callback, ctx);
 		/* Continue on error, make sure we get all acknowledgments */
-		if (ret == -1)
+		if (ret == -1) {
 			mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
+			err = -1;
+		}
 
 		ret = select(fd+1, &readfds, NULL, NULL, &tv);
 		if (ret == -1)
@@ -281,7 +284,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
 		FD_ZERO(&readfds);
 		FD_SET(fd, &readfds);
 	}
-	return ret;
+	return err;
 }
 
 int mnl_nft_rule_batch_add(struct nftnl_rule *nlr, struct nftnl_batch *batch,
-- 
2.1.4