On 5/4/20 3:26 AM, Paul Gofman wrote:
On 5/2/20 21:48, Zebediah Figura wrote:
Maybe this is too invasive or radical a change, but it strikes me as kind of weird that mapping_get_fd() [and mapping_fd_ops] exists, if the only purpose (at least, what seems to be the only purpose) is to pass the unix fd to virtual_map_section(). Maybe instead it would make more sense to pass that as part of the get_mapping_info request, and get rid of mapping fd objects altogether?
Yes, it seems like the only purpose of fd object for mapping is getting unix fd. But the unix fd for mapping is cacheable and the caching is currently managed by server_get_unix_fd() in ntdll through fd objects on server (I can guess that was the reason why fd object appeared for mapping at all, as server side does not seem to assume any file I/O on mapping objects). Do you think we should introduce a separate code path for mapping fds caching? Not that I think it is a big deal to do (we can still use the same cache and just replicate some logic from server_get_unix_fd()). But at the first glance it looks to me that we will trade a slight straightening up in mapping object handling (removing some fd type check) to partial duplication of caching logic. Please note also some checks for FD_TYPE is already have to be done for other cases, see, e. g., NtReadFileScatter().
Honestly, it's how I'd do it if it were me. But it's not me ;-)