On Fri May 17 08:36:29 2024 +0000, Jacek Caban wrote:
What I had in mind should work for that case, I think that it can be achieved by structuring the code differently. Right now you have object type specific getters for objects, that call common helpers, which then again need a switch for type-specific logic again. It additionally requires extra arguments like tid, which is also specific to the type (this MR just pretends it's useful), and is sub-optimal for things that don't need the whole thing id validation. I imagine that the lock function could be simplified to something like:
static NTSTATUS lock_shared_session_object( struct object_lock *lock, const object_shm_t *object, object_id_t id ) { if (lock->id == id && session_object_release_seqlock( object, lock->seq )) return STATUS_SUCCESS; session_object_acquire_seqlock( object, &lock->seq ); lock->id = id; return STATUS_PENDING; }And also:
UIT64 validate_lock_object( struct object_lock *lock, const object_shm_t *object ) { if (object && (!lock->id || lock->id == object->id)) return object->id; lock->id = 0; return 0; }And then type-specific getters can look like:
{ const shared_object_t *object = ...; /* get object from cache */ UINT64 id = validate_lock_object( lock, object ); if (!id) { /* object-specific code for getting offset and id (like a single case from find_shared_session_object) */ object = get_shared_session_pointer( offset ); } *ret = object; return lock_shared_session_object( lock, object, id ); }For things that don't need it, the additional `if()` could be simply skipped. Something like that seems cleaner to me, but I didn't try to actually implement it, so I may be missing something.
I have a hard time figuring whether this is correct or not wrt. id and locking consistency. Also, I still don't think it's a good idea that some object expect to be invalidated while other do not, when any object can be invalidated from the server side. It creates an unnecessary dependency between the client and the server code.
Anyway, I've pushed an update with all this code dropped for now, as in the current state it's only about desktop objects and the server doesn't need to invalidate anything. It still adds an assumption that desktop objects aren't destroyed while they have connected client, hopefully that's alright.