The branch, master has been updated via 00513da g_lock: Simplify g_lock_trylock via 6b3cc79 g_lock: Avoid a double call to serverid_exist from a98f09a selftest: Load time_audit and full_audit modules for all tests
https://git.samba.org/?p=samba.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit 00513daf78eb29996c81ca4d3be9e71a8e872829 Author: Volker Lendecke <v...@samba.org> Date: Mon Aug 13 15:07:06 2018 +0200 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 <v...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> Autobuild-User(master): Volker Lendecke <v...@samba.org> Autobuild-Date(master): Tue Aug 14 11:42:10 CEST 2018 on sn-devel-144 commit 6b3cc7916b81e001b886b9c632babe345ca21d76 Author: Volker Lendecke <v...@samba.org> Date: Mon Aug 13 14:12:47 2018 +0200 g_lock: Avoid a double call to serverid_exist If we try to G_LOCK_READ while a G_LOCK_WRITE is active, we do the serverid_exists call twice. Avoid that. Signed-off-by: Volker Lendecke <v...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> ----------------------------------------------------------------------- Summary of changes: source3/lib/g_lock.c | 56 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 19 deletions(-) Changeset truncated at 500 lines: diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c index bffbd6b..de24b6c 100644 --- 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; @@ -226,15 +225,20 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self, g_lock_get_rec(&lck, i, &check_rec); - if (!serverid_exists(&check_rec.pid)) { + if ((check_rec.lock_type == G_LOCK_READ) && + !serverid_exists(&check_rec.pid)) { g_lock_rec_del(&lck, i); modified = true; } } - 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); @@ -244,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; @@ -287,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)); -- Samba Shared Repository