On Fri Aug 4 14:41:49 2023 +0000, Jinoh Kang wrote:
Sorry, I've got a bit sidetracked. Let's return to the main discussion.
why we should get rid of this member.
The reason I brought this up is simple; it's not immediately clear to the reader that both of the following holds:
- We're lending this out to other server components.
- That lease is valid *only until* the mapping object is freed.
This leads to a few problems:
- If the server mapping object gets released too early, it'll take away
the mapped VMA with it while it's still borrowed. Later, someone may dereference the dangling mapping pointer, which points to unexpected memory. 2. In general, the caller of `create_shared_mapping()` has no control over the lifetime of the returned mapping pointer, making it less flexible. 3. It's extra overhead for every `struct mapping`, even if most sections aren't created via `create_shared_mapping()`. This `shared_ptr` member would be more easily justifiable if
- there were some kind of accessor like `void *mapping_get_shared_ptr(
struct mapping * );`, and
- the client component accesses the shared pointer only through it.
However, it's only ever used to be *freed*. In this case, I think it's better to let the caller of `create_shared_mapping()` free it instead. It gives a more clear picture of ownership management.
The callers hold a reference to the mapping object, and use its mapped pointer. The pointer stays valid as long as they keep a reference on the mapping object. The ownership seems clear to me, and it's simpler than having to manage the pointer lifetime separately which would require every user to munmap the pointer when they're done.
Having the lifetime of the mapping and the pointer separate also makes it unclear how they are related together, and you could keep using the mapped pointer with the mapping reference released, which imho would be incorrect.