IOS HLE: Deduplicate the request parsing code #4661
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leoetlino
force-pushed
the
ios-request
branch
4 times, most recently
from
45ba37c
to
b4a9f88
Compare
BhaaLseN
reviewed
Jan 16, 2017
| { | ||
| // async | ||
| m_event.addr = _CommandAddress; | ||
| Memory::Write_U32(0, _CommandAddress + 0x4); | ||
| // Check if the condition is already true | ||
| EventNotify(); | ||
| return GetNoReply(); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| u32 fd = 0; | ||
| explicit IOSRequest(u32 address); | ||
| virtual ~IOSRequest() = default; | ||
| void SetReturnValue(s32 new_return_value) const; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| { | ||
| struct IOVector | ||
| { | ||
| u32 addr = 0; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This adds well-defined structs that are responsible for parsing resource requests, instead of duplicating the logic and offsets all over IOS HLE. Command handler functions are now passed parsed requests instead of a command address. This may not seem like a very important change, but it removes the need to remember all of the struct offsets or copy/paste existing struct request variables. It also prevents nasty bugs which have occurred in the past, such as parsing an ioctl as if it were an ioctlv (that's way too easy to do if you pass command addresses directly); or writing something to 0x0, which can easily happen by mistake with a close handler that can be called with invalid command addresses. Bonus changes: - The return code is not an obscure Memory::Write_U32 anymore, but an explicit, more obvious SetReturnValue() call. (Which correctly takes a s32 instead of a u32, since return codes are signed.) - Open handlers are now only responsible for returning an IOS ret code, and Close handlers don't return anything and don't have to worry about checking whether the request is a real one anymore. - DumpAsync was moved to the ioctlv request struct, because it did not really make sense to make it part of the IOS device and it only works for ioctlvs. All current usages have been removed; they will be readded in a later commit. As of this commit, nothing uses the structs yet. Usages will be migrated progressively.
Now that everything has been changed to use the new structs, the old methods and structs can be removed. And while I was changing the base device class, I also moved the "unsupported command" code to a separate function. It was pretty silly to copy the same 3 lines for ~5 commands.
This makes more sense than setting the return code on the request struct first before replying. Ref: dolphin-emu#4661 (comment)
leoetlino
force-pushed
the
ios-request
branch
from
b4a9f88
to
25f89cc
Compare
|
@BhaaLseN I've made the requested changes. |
Parlane
approved these changes
Jan 16, 2017
BhaaLseN
approved these changes
Jan 17, 2017
|
|
||
| IPCCommandResult reply = GetDefaultReply(); | ||
| IPCCommandResult reply = GetDefaultReply(IPC_SUCCESS); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves the resource request parsing code into well-defined structs instead of duplicating it all over IOS HLE. Command handler functions are now passed parsed resource requests instead of a command address.
This may not seem like a very important change, but it removes the need to remember all of the struct offsets or more likely, copy/paste existing struct request variables. It also makes it harder to introduce nasty bugs which have occurred in the past, such as parsing an ioctl as if it were an ioctlv (that's way too easy to do if you pass command addresses directly); or writing something to 0x0, which can easily happen by mistake with a close handler that can be called with invalid command addresses.
Some other notable changes:
The return code is not an obscure Memory::Write_U32 anymore, but an explicit, more obvious SetReturnValue() call. (Which correctly takes a s32 instead of a u32, since return codes are signed.)
Open handlers are now only responsible for returning an IOS return code, and Close handlers don't return anything and don't have to worry about checking whether the request is a real one anymore.
Dumpwas moved to the relevant request structs, because it did not really make sense to make it part of the IOS device especially when it only works for ioctl/ioctlv requests.This was split from #4488 so it has already been tested by JMC47.