From: Volker Lendecke <vl@samba.org>
Date: Wed, 24 Jul 2019 15:39:59 +0000 (+0200)
Subject: smbd: Simplify fd_open_atomic()
X-Git-Tag: tdb-1.4.2~180
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=fe26ff6e911f835808ac3f465822f86260407020;p=thirdparty%2Fsamba.git

smbd: Simplify fd_open_atomic()

* Assign *file_created on every exit.

* Directly assign curr_flags without &= / |=

Both of these changes make the routine easier to understand for me,
less jumping around in the code to see where the values came from.

* Do the retry in a "positive" if-clause

Normally I'm a big fan of early returns, but this single retry is so
simple that to me it's easier to understand this way.

Overall, 13 lines less code. YMMV :-)

Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

Autobuild-User(master): Jeremy Allison <jra@samba.org>
Autobuild-Date(master): Sat Aug 10 00:07:28 UTC 2019 on sn-devel-184
---

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index faba16a7fef..13a7d7237c7 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1051,13 +1051,13 @@ static NTSTATUS fd_open_atomic(struct connection_struct *conn,
 	bool file_existed = VALID_STAT(fsp->fsp_name->st);
 	int curr_flags;
 
-	*file_created = false;
-
 	if (!(flags & O_CREAT)) {
 		/*
 		 * We're not creating the file, just pass through.
 		 */
-		return fd_open(conn, fsp, flags, mode);
+		status = fd_open(conn, fsp, flags, mode);
+		*file_created = false;
+		return status;
 	}
 
 	if (flags & O_EXCL) {
@@ -1096,50 +1096,37 @@ static NTSTATUS fd_open_atomic(struct connection_struct *conn,
 	 * mapped from the ELOOP POSIX error.
 	 */
 
-	curr_flags = flags;
-
 	if (file_existed) {
-		curr_flags &= ~(O_CREAT);
+		curr_flags = flags & ~(O_CREAT);
 		retry_status = NT_STATUS_OBJECT_NAME_NOT_FOUND;
 	} else {
-		curr_flags |= O_EXCL;
+		curr_flags = flags | O_EXCL;
 		retry_status = NT_STATUS_OBJECT_NAME_COLLISION;
 	}
 
 	status = fd_open(conn, fsp, curr_flags, mode);
 	if (NT_STATUS_IS_OK(status)) {
-		if (!file_existed) {
-			*file_created = true;
-		}
+		*file_created = !file_existed;
 		return NT_STATUS_OK;
 	}
-	if (!NT_STATUS_EQUAL(status, retry_status)) {
-		return status;
-	}
+	if (NT_STATUS_EQUAL(status, retry_status)) {
 
-	curr_flags = flags;
+		file_existed = !file_existed;
 
-	/*
-	 * Keep file_existed up to date for clarity.
-	 */
-	if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
-		file_existed = false;
-		curr_flags |= O_EXCL;
-		DBG_DEBUG("file %s did not exist. Retry.\n",
-			smb_fname_str_dbg(fsp->fsp_name));
-	} else {
-		file_existed = true;
-		curr_flags &= ~(O_CREAT);
-		DBG_DEBUG("file %s existed. Retry.\n",
-			smb_fname_str_dbg(fsp->fsp_name));
-	}
+		DBG_DEBUG("File %s %s. Retry.\n",
+			  fsp_str_dbg(fsp),
+			  file_existed ? "existed" : "did not exist");
 
-	status = fd_open(conn, fsp, curr_flags, mode);
+		if (file_existed) {
+			curr_flags = flags & ~(O_CREAT);
+		} else {
+			curr_flags = flags | O_EXCL;
+		}
 
-	if (NT_STATUS_IS_OK(status) && (!file_existed)) {
-		*file_created = true;
+		status = fd_open(conn, fsp, curr_flags, mode);
 	}
 
+	*file_created = (NT_STATUS_IS_OK(status) && !file_existed);
 	return status;
 }