mame icon indicating copy to clipboard operation
mame copied to clipboard

util/ioprocs.cpp: Add wrappers for common patterns

Open cuavas opened this issue 2 years ago • 6 comments

First, some background on this. POSIX-like read/write operations have somewhat complicated semantics. In particular, they can return short for multiple reasons:

  • If a read or write call is interrupted by an asynchronous signal before completing, it will return short. If no data has been read/written it will set EINTR, otherwise it will return success.
  • If a read or write to a non-blocking descriptor would block, it will return short. If no data has been read/written, it will set EAGAIN or EWOULDBLOCK, otherwise it will return success.
  • If some characteristic of the underlying device would prevent a read/write of the requested size from appearing to be atomic with respect to other read/write calls, it will return short.
  • A read will return short if the end of the stream is reached. However the application must attempt to complete the read to distinguish between a read that failed to complete for some other reason (e.g. EAGAIN) and actually reaching the end of the stream.
  • If a write partially completes before encountering an error, it will return short. To obtain the actual error, you must attempt to complete the write, which means the error is not obtained atomically. This can lead to results that are not intuitive. For example, if a write partially completes because the device runs out of free space, but more space is freed before the application attempts to complete the write, the application will see the write complete (albeit not atomically with respect to other read/write calls), never seeing the ENOSPC or EDQUOT error.

Reading/writing zero bytes may cause error checking to be performed, so it may fail in some cases. As such, attempting to read/write zero bytes should not be simply ignored.

This adds try_read/try_read_at and try_write/try_write_at wrappers to encapsulate the process of attempting to complete a read or write for cases where one doesn’t care whether the operation is atomic with respect to other reads/writes.

The functions return pairs or tuples so they can be used with structured binding declarations and std::tie when convenient. They’re declared as free functions to keep the interfaces pure. They can be called using unqualified names due to argument-dependent name lookup.

There are variants of try_read and try_read_at that allocate space for the output. Note that they will always allocate the requested number of bytes even though fewer bytes may actually be read. One limitation of the allocating variants is that they won’t work if you try to read zero bytes for the purpose of checking for errors – the non-allocating variants must be used for this purpose (or you can call read directly on the stream).

I’m not convinced try_read and try_write are necessarily the best names for these. If someone can come up with unambiguously better names, I’d be happy to change them.

Like #11588, this also removes some device_image_interface member functions in favour of working with the image file object directly.

I haven’t ported much code across to use these functions, since the names may need to change.

cuavas avatar Oct 11 '23 17:10 cuavas

I’m pretty tired, so I could have screwed something up here. I’ll definitely look over this again myself.

cuavas avatar Oct 11 '23 17:10 cuavas

Some initial comments:

  • Yes, I don't particularly like the try_read and try_write names, which aren't very suggestive of how they should be used. However, read_block and write_block are the best names I could come up with.
  • I don't think it's such a good idea to give the same names to functions that allocate memory and functions that require a block of previously allocated memory. At least, unlike with device_image_interface::fread and device_image_interface::fwrite, these don't have the same number of arguments, so they might be less easy to confuse.

PS:

  • Is there any particular reason other than purism for making these free functions rather than class members? If they're also declared in ioprocs.h, there's no real practical benefit.

ajrhacker avatar Oct 12 '23 16:10 ajrhacker

I also don't like the naming, but I don't immediately have a better suggestion either. Other than that, I like it.

rb6502 avatar Oct 13 '23 14:10 rb6502

OK, so if I went with what @ajrhacker is suggesting, would the names be:

  • read_block
  • read_block_at
  • alloc_read_block
  • alloc_read_block_at
  • write_block
  • write_block_at

My concerns with that would be that they could be misinterpreted as involving blocking I/O (potentially causing someone to believe that regular read and write never block), and also that they actually potentially do multiple read/write operations which could potentially be seen as multiple “blocks”.

Another alternative would be to just call these ones read and write and rename the ones on the interface to read_some and write_some (analogous to the convention used by Boost libraries), but ain’t nobody got time for that.

@galibert do you have opinions on naming?

cuavas avatar Oct 16 '23 18:10 cuavas

Overloading of simple terminology has been the curse of computing jargon since the 1950s. I might suggest read/write_chunk as an alternative naming suggestion to read/write_block.

ajrhacker avatar Oct 25 '23 23:10 ajrhacker

@galibert can I get an opinion on the names?

Should I bite the bullet and change the member functions to read_some/write_some and change the helper wrappers to read/write to make it analogous to boost?

cuavas avatar Dec 14 '23 14:12 cuavas

Updated to change the interface functions to read_some/write_some and the wrappers to read/write to make it clearer that the ones with “some” in the name may read or write less than requested.

cuavas avatar Feb 22 '24 12:02 cuavas