Another set of fixes for returning -1 as bool

Rikard Falkeborn rikard.falkeborn at gmail.com
Sat May 18 05:40:09 UTC 2019


Returning -1 from a function with bool as return value type is usually not
correct. This follows up on commit 749f1290ce6 (
https://lists.samba.org/archive/samba-technical/2019-May/133542.html) and
fixes the remaining issues of this kind spotted by cppcheck.

I'm pretty sure about the first three patches. The last one basically
inverts the return values of the test (except in one error path), which
means either it has never worked as intended, or I have really
misunderstood tthings completely.

MR: https://gitlab.com/samba-team/samba/merge_requests/460
CI: https://gitlab.com/rikardfalkeborn/samba/pipelines/61911098

Please review.
-------------- next part --------------
From 6459ed7ff0cd0add4144bf08635fb4943131041d Mon Sep 17 00:00:00 2001
From: Rikard Falkeborn <rikard.falkeborn at gmail.com>
Date: Thu, 16 May 2019 21:21:11 +0200
Subject: [PATCH 1/4] vfs_catia: Fix return value in lock functions

Returning -1 in a function with bool as return value type is the same
as returning true. Change to false to indicate the error.

Detected by the help of cppcheck.

Signed-off-by: Rikard Falkeborn <rikard.falkeborn at gmail.com>
---
 source3/modules/vfs_catia.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/source3/modules/vfs_catia.c b/source3/modules/vfs_catia.c
index c362be764cc..5915e40ff28 100644
--- a/source3/modules/vfs_catia.c
+++ b/source3/modules/vfs_catia.c
@@ -2116,7 +2116,7 @@ static bool catia_lock(vfs_handle_struct *handle,
 
 	ret = CATIA_FETCH_FSP_PRE_NEXT(talloc_tos(), handle, fsp, &cc);
 	if (ret != 0) {
-		return -1;
+		return false;
 	}
 
 	ok = SMB_VFS_NEXT_LOCK(handle, fsp, op, offset, count, type);
@@ -2178,7 +2178,7 @@ static bool catia_getlock(vfs_handle_struct *handle,
 
 	ret = CATIA_FETCH_FSP_PRE_NEXT(talloc_tos(), handle, fsp, &cc);
 	if (ret != 0) {
-		return -1;
+		return false;
 	}
 
 	ok = SMB_VFS_NEXT_GETLOCK(handle, fsp, poffset, pcount, ptype, ppid);
@@ -2198,7 +2198,7 @@ static bool catia_strict_lock_check(struct vfs_handle_struct *handle,
 
 	ret = CATIA_FETCH_FSP_PRE_NEXT(talloc_tos(), handle, fsp, &cc);
 	if (ret != 0) {
-		return -1;
+		return false;
 	}
 
 	ok = SMB_VFS_NEXT_STRICT_LOCK_CHECK(handle, fsp, plock);
-- 
2.21.0


From ad71550f849d7147b26e531213e198aaa3e3732b Mon Sep 17 00:00:00 2001
From: Rikard Falkeborn <rikard.falkeborn at gmail.com>
Date: Thu, 16 May 2019 21:29:52 +0200
Subject: [PATCH 2/4] vfs_gpfs: Fix return value if getting data fails

Returning -1 in a function with bool as return value type is the same
as returning true. Change to false to indicate the error.

Detected by the help of cppcheck.

Signed-off-by: Rikard Falkeborn <rikard.falkeborn at gmail.com>
---
 source3/modules/vfs_gpfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
index 52c4e5ef25d..a1fe91d0df4 100644
--- a/source3/modules/vfs_gpfs.c
+++ b/source3/modules/vfs_gpfs.c
@@ -1995,7 +1995,7 @@ static bool vfs_gpfs_is_offline(struct vfs_handle_struct *handle,
 
 	SMB_VFS_HANDLE_GET_DATA(handle, config,
 				struct gpfs_config_data,
-				return -1);
+				return false);
 
 	if (!config->winattr) {
 		return false;
-- 
2.21.0


From 034a5fc33a1478a844a73d454b53f9e694c7e2d3 Mon Sep 17 00:00:00 2001
From: Rikard Falkeborn <rikard.falkeborn at gmail.com>
Date: Thu, 16 May 2019 21:03:42 +0200
Subject: [PATCH 3/4] s3: libsmbclient: Fix return value if cli_open() fails

Returning -1 in a function with bool as return value type is the same
as returning true. Change to false to indicate the error.

Detected by the help of cppcheck.

Signed-off-by: Rikard Falkeborn <rikard.falkeborn at gmail.com>
---
 source3/libsmb/libsmb_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source3/libsmb/libsmb_file.c b/source3/libsmb/libsmb_file.c
index ebd0bfe422a..be9bcd936b2 100644
--- a/source3/libsmb/libsmb_file.c
+++ b/source3/libsmb/libsmb_file.c
@@ -619,7 +619,7 @@ SMBC_setatr(SMBCCTX * context, SMBCSRV *srv, char *path,
                 if (!NT_STATUS_IS_OK(cli_open(srv->cli, path, O_RDWR, DENY_NONE, &fd))) {
                         errno = SMBC_errno(context, srv->cli);
 			TALLOC_FREE(frame);
-                        return -1;
+                        return False;
                 }
 
                 /* Set the new attributes */
-- 
2.21.0


From b13c4bbbff56bb02f906aa70fa550dfdcbe1372e Mon Sep 17 00:00:00 2001
From: Rikard Falkeborn <rikard.falkeborn at gmail.com>
Date: Thu, 16 May 2019 21:43:46 +0200
Subject: [PATCH 4/4] s3: torture: Fix return values

Torture tests should return true on success and false on failure.
Returning -1 is the same as returning true and returning 0 is the same
as returning false. Change the return values to true and false to fix
the return values.

Detected by the help of cppcheck.

Signed-off-by: Rikard Falkeborn <rikard.falkeborn at gmail.com>
---
 source3/torture/test_addrchange.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/source3/torture/test_addrchange.c b/source3/torture/test_addrchange.c
index daf0488aa18..9ccca1c6c51 100644
--- a/source3/torture/test_addrchange.c
+++ b/source3/torture/test_addrchange.c
@@ -34,7 +34,7 @@ bool run_addrchange(int dummy)
 	ev = samba_tevent_context_init(talloc_tos());
 	if (ev == NULL) {
 		d_fprintf(stderr, "tevent_context_init failed\n");
-		return -1;
+		return false;
 	}
 
 	status = addrchange_context_create(talloc_tos(), &ctx);
@@ -54,14 +54,14 @@ bool run_addrchange(int dummy)
 		req = addrchange_send(talloc_tos(), ev, ctx);
 		if (req == NULL) {
 			d_fprintf(stderr, "addrchange_send failed\n");
-			return -1;
+			return false;
 		}
 
 		if (!tevent_req_poll_ntstatus(req, ev, &status)) {
 			d_fprintf(stderr, "tevent_req_poll_ntstatus failed: "
 				  "%s\n", nt_errstr(status));
 			TALLOC_FREE(req);
-			return -1;
+			return false;
 		}
 
 		status = addrchange_recv(req, &type, &addr);
@@ -69,7 +69,7 @@ bool run_addrchange(int dummy)
 		if (!NT_STATUS_IS_OK(status)) {
 			d_fprintf(stderr, "addrchange_recv failed: %s\n",
 				  nt_errstr(status));
-			return -1;
+			return false;
 		}
 
 		switch(type) {
@@ -90,5 +90,5 @@ bool run_addrchange(int dummy)
 	}
 	TALLOC_FREE(ctx);
 	TALLOC_FREE(ev);
-	return 0;
+	return true;
 }
-- 
2.21.0



More information about the samba-technical mailing list