On Fri May 17 08:36:29 2024 +0000, Rémi Bernon wrote:
Sure, we can probably try to initialize the desktop / thread message queue cached objects from the requests that change them, but I don't think it's a good idea to treat these objects specifically otherwise. Objects can be invalidated from the server side by simply changing their id, and clients then have to call wineserver to get the new object id / offset. Of course, right now desktop / queues objects never need to change, but we can very well imagine doing something differently, and I think there's no reason to introduce a different handling on the client side depending on the object types. Fwiw the full branch is available here: https://gitlab.winehq.org/rbernon/wine/-/tree/wip/shared-memories
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.