Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util/ioprocs.cpp: Add wrappers for common patterns #11608

Merged
merged 2 commits into from Feb 24, 2024

Conversation

cuavas
Copy link
Member

@cuavas cuavas commented Oct 11, 2023

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
Copy link
Member Author

cuavas commented Oct 11, 2023

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

@ajrhacker
Copy link
Contributor

ajrhacker commented Oct 12, 2023

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.

@rb6502
Copy link
Contributor

rb6502 commented Oct 13, 2023

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

@cuavas
Copy link
Member Author

cuavas commented Oct 16, 2023

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?

@ajrhacker
Copy link
Contributor

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.

@cuavas
Copy link
Member Author

cuavas commented Dec 14, 2023

@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?

emu/diimage.h: Removed fread overloads that allocate memory for output.

util/core_file.cpp: Changed output size of load to size_t.
@cuavas
Copy link
Member Author

cuavas commented Feb 22, 2024

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 cuavas merged commit 1615b85 into mamedev:master Feb 24, 2024
0 of 5 checks passed
@cuavas cuavas deleted the iowrappers branch February 24, 2024 15:29
Mokona pushed a commit to Mokona/mame that referenced this pull request Feb 28, 2024
emu/diimage.h: Removed fread overloads that allocate memory for output.

util/core_file.cpp: Changed output size of load to size_t.
stonedDiscord pushed a commit to stonedDiscord/mame that referenced this pull request Apr 8, 2024
emu/diimage.h: Removed fread overloads that allocate memory for output.

util/core_file.cpp: Changed output size of load to size_t.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants