Hello, On Mon, Jan 29, 2024 at 11:59 PM Samuel Thibault <samuel.thiba...@gnu.org> wrote: > Please notably review the RPC part, I really don't know that much about > mig.
Some nitpicks inline. Flávio, does what I'm saying below make sense to you? > +/* > + * vm_pages_phys returns information about a region of memory > + */ > +kern_return_t vm_pages_phys( > + host_t host, > + vm_map_t map, > + vm_address_t address, > + vm_size_t size, This is supposed to be a number of pages rather than VM region size, right? Then it would be better off with a different name, and perhaps type. Alternatively perhaps it _should_ be a VM region size? > + rpc_phys_addr_array_t *pagespp, > + mach_msg_type_number_t *countp) > +{ > + if (host == HOST_NULL) > + return KERN_INVALID_HOST; > + if (map == VM_MAP_NULL) > + return KERN_INVALID_TASK; > + > + if (!page_aligned(address)) > + return KERN_INVALID_ARGUMENT; > + if (!page_aligned(size)) > + return KERN_INVALID_ARGUMENT; > + > + mach_msg_type_number_t count = atop(size), cur; > + > + if (*countp < count) > + return KERN_INVALID_ARGUMENT; It's not an error if the passed-in buffer is smaller than what you're planning to return. This is not even something that the user side of the RPC controls, the buffer is fixed-size and allocated by MIG in the reply message -- unless you use CountInOut, that is. You're supposed to allocate your own buffer in this case, something like this: rpc_phys_addr_array_t pages = *pagespp; if (*countp < count) { vm_offset_t allocated; kr = kmem_alloc(ipc_kernel_map, &allocated, some_size); if (kr != KERN_SUCCESS) return kr; pages = (rpc_phys_addr_array_t) allocated; } and at the end: if (pages != *pagespp) { vm_map_copy_t copy; kr = vm_map_copyin(ipc_kernel_map, (vm_offset_t) pages, some_size, TRUE, ©); assert(kr == KERN_SUCCESS); *pagespp = (rpc_phys_addr_array_t) copy; } *countp = count; > + *countp = count; > + > + return KERN_SUCCESS; > +} OK. My current understanding is that you indeed do not need to deallocate either the 'vm_map_t map' argument or the task port right behind it on the success path here. But perhaps it would be clearer with a comment stating that this is intentional (and not simply forgotten). Sergey