Shared memory API review

[marmarek]

https://github.com/shawnanastasio/libkvmchan/blob/ff1e221fa5eb39f37abf62a0a9c50de1d381d7bc/libkvmchan.h#L72-L118

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