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 faba16a7fef445d75bd6425163c6f876b18eacab..13a7d7237c7aaf54f5ee8677d8f8536916fdd249 100644 (file)
--- 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;
 }