g_lock: Simplify g_lock_trylock

While chasing a bug in g_lock (not in master) I saw some opportunity to
simplify g_lock_trylock a bit. This is array handling, and array
handling is just extremely error-prone. This *might* be a little less
efficient or large numbers of READ locks, but this remains to be
seen. For now, simplify the code.

First, we make two passes now: One to remove ourselves, and the other
one to search for conflicts. Mixing up both made it pretty hard for me
to follow the code.

Second, I've removed the _mylock and mylock pointer/struct logic and
replaced it with the "mylock.pid.pid != 0 ? &mylock : NULL" when calling
g_lock_store. To me, this focuses the logic whether to add ourselves in
one place instead of spreading it around in the whole routine.

Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Autobuild-User(master): Volker Lendecke <vl@samba.org>
Autobuild-Date(master): Tue Aug 14 11:42:10 CEST 2018 on sn-devel-144

diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
index c824f4b0a587110ffd13952cbbac647da7df5485..de24b6c847b288f279772b1add470ddf9406292c 100644 (file)
--- a/source3/lib/g_lock.c
+++ b/source3/lib/g_lock.c
@@ -200,8 +200,7 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
        TDB_DATA data;
        size_t i;
        struct g_lock lck;
-       struct g_lock_rec _mylock;
-       struct g_lock_rec *mylock = NULL;
+       struct g_lock_rec mylock = {0};
        NTSTATUS status;
        bool modified = false;
        bool ok;
@@ -233,9 +232,13 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
                }
        }
 
-       i = 0;
+       /*
+        * For the lock upgrade/downgrade case, remove ourselves from
+        * the list. We re-add ourselves later after we checked the
+        * other entries for conflict.
+        */
 
-       while (i < lck.num_recs) {
+       for (i=0; i<lck.num_recs; i++) {
                struct g_lock_rec lock;
 
                g_lock_get_rec(&lck, i, &lock);
@@ -245,20 +248,26 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
                                status = NT_STATUS_WAS_LOCKED;
                                goto done;
                        }
-                       if (mylock != NULL) {
-                               status = NT_STATUS_INTERNAL_DB_CORRUPTION;
-                               goto done;
-                       }
-                       _mylock = lock;
-                       mylock = &_mylock;
-                       /*
-                        * Remove "our" lock entry. Re-add it later
-                        * with our new lock type.
-                        */
+
+                       mylock = lock;
                        g_lock_rec_del(&lck, i);
                        modified = true;
-                       continue;
+                       break;
                }
+       }
+
+       /*
+        * Check for conflicts with everybody else. Not a for-loop
+        * because we remove stale entries in the meantime,
+        * decrementing lck.num_recs.
+        */
+
+       i = 0;
+
+       while (i < lck.num_recs) {
+               struct g_lock_rec lock;
+
+               g_lock_get_rec(&lck, i, &lock);
 
                if (g_lock_conflicts(type, lock.lock_type)) {
                        struct server_id pid = lock.pid;
@@ -288,18 +297,26 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
 
        modified = true;
 
-       _mylock = (struct g_lock_rec) {
+       mylock = (struct g_lock_rec) {
                .pid = self,
                .lock_type = type
        };
-       mylock = &_mylock;
 
        status = NT_STATUS_OK;
 done:
        if (modified) {
                NTSTATUS store_status;
 
-               store_status = g_lock_store(rec, &lck, mylock);
+               /*
+                * (Re-)add ourselves if needed via non-NULL
+                * g_lock_store argument
+                */
+
+               store_status = g_lock_store(
+                       rec,
+                       &lck,
+                       mylock.pid.pid != 0 ? &mylock : NULL);
+
                if (!NT_STATUS_IS_OK(store_status)) {
                        DBG_WARNING("g_lock_record_store failed: %s\n",
                                    nt_errstr(store_status));