Shared memory API review

[marmarek]

libkvmchan/libkvmchan.h

Lines 72 to 118 in ff1e221

	/**
	* Allocate memory to share with the provided domain.
	*
	* @param handle previously opened `libkvmchan_shmem` handle
	* @param client_dom domain # to share memory with
	* @param page_count number of shared pages to allocate
	* @param[out] region_id_out allocated region ID for new shared memory
	* @return pointer to allocated shared memory region, or NULL on failure
	*/
	LIBKVMCHAN_EXPORTED void *libkvmchan_shmem_region_create(struct libkvmchan_shmem *handle, uint32_t client_dom,
	uint32_t page_count, uint32_t *region_id_out);
	/**
	* Connect to and map a shared memory region created by the provided domain.
	*
	* @param handle previously opened `libkvmchan_shmem` handle
	* @param server_dom domain # of server that allocated the region
	* @param region_id ID of shmem region to connect to
	* @return pointer to shared memory region, or NULL on failure
	*/
	LIBKVMCHAN_EXPORTED void *libkvmchan_shmem_region_connect(struct libkvmchan_shmem *handle, uint32_t server_dom, uint32_t region_id);
	/**
	* Close a previously opened shared memory region.
	*
	* In the case of a server, the region will no longer be mappable by the client domain after close,
	* but existing client mappings will still be open.
	*
	* In the case of a client, the region will be unmapped locally but can be re-mapped again so long as
	* the server does not close the region.
	*
	* @param handle previously opened `libkvmchan_shmem` handle
	* @param ptr pointer to opened shared memory region, obtained from either `libkvmchan_shmem_region_create`
	* OR `libkvmchan_shmem_region_connect`
	* @return 0 on success, -1 on failure
	*/
	LIBKVMCHAN_EXPORTED int libkvmchan_shmem_region_close(struct libkvmchan_shmem *handle, void *ptr);
	/**
	* Same as `libkvmchan_shmem_region_close` but accepts a peer domain number and region ID instead of
	* a pointer to the mapped region.
	*
	* @param handle previously opened `libkvmchan_shmem` handle
	* @param peer_dom domain # that this region is shared with
	* @param region_di ID of the shared region
	*/
	LIBKVMCHAN_EXPORTED int libkvmchan_shmem_region_close_by_id(struct libkvmchan_shmem *handle, uint32_t peer_dom, uint32_t region_id);

Is region_id (in all of those functions) a single ID for the whole region, or separate for each page? This needs to be clarified in a comment. In either case, some adjustments are needed:
In case of a single ID:

• libkvmchan_shmem_region_connect needs to return how much memory was mapped (or take expected size and return an error if it doesn't match the region size)
  In case of one ID per page:
• libkvmchan_shmem_region_connect should support mapping several pages at once, into a continuous mapping
• consider "close" functions handling multiple pages at once too

Futhermore:

• libkvmchan_shmem_region_close - clarify what happens if ptr does not belong to a shmem region, or points in the middle of one (instead of a the beginning)
• what happens if the same region is mapped several times? What libkvmchan_shmem_region_close_by_id does then?

[shawnanastasio]

Is region_id (in all of those functions) a single ID for the whole region, or separate for each page? This needs to be clarified in a comment.

It is for the entire region - I'll update the comment to clarify this.

In either case, some adjustments are needed:
In case of a single ID:

• libkvmchan_shmem_region_connect needs to return how much memory was mapped (or take expected size and return an error if it doesn't match the region size)

OK, sounds reasonable. I'm leaning towards returning the size mapped via another out pointer but if you think taking an expected size is preferable I can do that too.

Futhermore:

• libkvmchan_shmem_region_close - clarify what happens if ptr does not belong to a shmem region, or points in the middle of one (instead of a the beginning)

As it stands libkvmchan_shmem_region_close will reject any ptr that does not exactly match what was returned by libkvmchan_shmem_region_create or libkvmchan_shmem_region_connect. I'll update the comment to clarify this.

• what happens if the same region is mapped several times? What libkvmchan_shmem_region_close_by_id does then?

Currently the host kvmchand daemon will reject attempts to map an already mapped region, so libkvmchan_shmem_region_connect can only be called once (unless it is closed, then it can be mapped again).

Is supporting multiple concurrent mappings of the same region a requirement? If so I'll probably just remove the close_by_id API.

[shawnanastasio]

I've updated the comments for libkvmchan_shmem_region_create as well as libkvmchan_shmem_region_close to address your comments

[marmarek]

Is supporting multiple concurrent mappings of the same region a requirement?

No, it just needed to be clarified (and the actual implementation should either support it or refuse mapping, not crash ;) ).

[shawnanastasio]

No, it just needed to be clarified (and the actual implementation should either support it or refuse mapping, not crash ;) ).

Ok, thanks for clarifying. The current behavior is indeed to refuse mapping. I'll update the comment