-
-
Notifications
You must be signed in to change notification settings - Fork 10
Fix file write time not being emulated after all handles were close, update script tools to fix bugs in bf emulator #26
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
Fix file write time not being emulated after all handles were close, update script tools to fix bugs in bf emulator #26
Conversation
|
I'll get this sorted alongside the other PR tomorrow then, my day will be packed today. |
| fileInfo = new(newFilePath, 0, emulatedFile); | ||
| HandleToInfoMap[hndl] = fileInfo; | ||
| PathToHandleMap[newFilePath] = hndl; | ||
| PathToVirtualFileMap[newFilePath] = fileInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved a bit hastily, this and last time.
Actually, can you run me through this? @AnimatedSwine37
PathToVirtualFileMap[newFilePath] = fileInfo;
This sets a RegisterVirtualFile mapping (essentially) when a file is emulated.
for
FileEmulationFramework/FileEmulationFramework/FileAccessServer.cs
Lines 153 to 185 in c95ac26
| if (!PathToHandleMap.TryGetValue(path, out var hfile) || !HandleToInfoMap.TryGetValue(hfile, out var info)) | |
| { | |
| // We haven't tried to create an emulated file for this yet, try it | |
| if (!PathToVirtualFileMap.TryGetValue(path, out info)) | |
| { | |
| // Prevent recursion | |
| PathToVirtualFileMap[path] = null; | |
| // There is a virtual file but no handle exists for it, we need to make one | |
| try | |
| { | |
| safeHandle = File.OpenHandle(path); | |
| } | |
| catch (Exception e) | |
| { | |
| // If we couldn't make the handle this probably isn't a file we can emulate | |
| return result; | |
| } | |
| hfile = safeHandle.DangerousGetHandle(); | |
| // We tried to make one but failed, this file isn't emulated | |
| if (!HandleToInfoMap.TryGetValue(hfile, out info)) | |
| { | |
| safeHandle.Close(); | |
| return result; | |
| } | |
| } | |
| // We've tried to create an emulated file for this before but failed, this file isn't emulated | |
| if (info == null) | |
| return result; | |
| } |
To pick up.
But this check is not made in QueryAttributesFileImpl?
That's one part of it.
The other part is, PathToVirtualFileMap is supposed to be used for manual overrides via API. Setting this upon file emulation happening is a bit of a hack. That's because it relies on the fact that opening the file will always create the same result, i.e. that it is deterministic.
That's technically not always true (sometimes it's not deterministic). If a mod queries the file, then registers a file in the place, and then opens it.
I presume the case of how this came up is:
- You opened a file, which got emulated.
- New emulated file has a new timestamp as result
- Somewhere down the road the emulated file is queried without opening a handle.
In any case, we're introducing a bug by fixing another bug; as we don't have any strict 'rules' here. That should be documented, via issue and/or in the code. An example rule could be that a file, once emulated will always be deterministic unless invalidated via API.
This will need thinking through in the Rust rewrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It not being checked in QueryAttributesFileImpl is just a mistake, I didn't notice that a path was used there as well.
I guess I just got (un)lucky and the code that was checking last write time and file size always ended up using QueryFullAttributesFile so I didn't notice.
The case this was trying to solve is:
- The last write time or size is queried before an emulated file is ever actually created
- Because it hasn't been created we have to create it so you get the emulated file's info, not the original
I didn't think about the emulated file not being deterministic. I'm not sure if it actually would be possible to handle that when it's being queried by path like in these functions. For example, say:
- File X is created, making handle A and emulated file A which is associated with the path for file X
- File X is created again (old one is still open) it makes handle B and emulated file B
- I query the path of file X. Now, which handle did I actually want, A or B? I don't think it's possible to say knowing just the path
I think for now your example of saying the emulated file is deterministic unless explicitly invalidated would work. The existing emulators all already have code like:
if (_pathToEmulated.TryGetValue(filepath, out var emulatedBf))
{
// Avoid recursion into same file.
if (emulatedBf == null)
return false;
emulated = new EmulatedFile<Stream>(emulatedBf.Stream, emulatedBf.LastWriteTime);
return true;
}Where they're just using the existing emulated file if one has already been made for a particular path so I think this rule would already be satisfied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, pretty much; you got it there on the money.
Technically it's possible to query a file, making an emulated file, then open the file afterward; but the emulators do somewhat guard against this, as in the very example you linked, so we're good on that part.
Sewer56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I got 2 requests:
- Don't use
PathToVirtualFileMapfor previously emulated files.- Since that is for API use only.
- Handle the missing file case in
QueryAttributesFileImplfor whenPathToVirtualFileMapis set.
Note that in DequeueHandles, we only dequeue from HandleToInfoMap, and not PathToHandleMap. In other words, you can check PathToHandleMap to determine if a file was previously emulated. Use that instead of using PathToVirtualFileMap.
|
Anyway, I didn't review the last PR properly, and it actually broke trimming (and building 2 mods), so I'm going to fix that on my own end quickly. |
…eated, use PathToHandleMap instead of PathToVirtualFileMap to emulate attributes of new files
|
I think that should be good now, lmk if there's anything else |
|
Looks good to me 👍 |
|
I also pushed a release for you, after bumping the project versions. |
This pr just has two small fixes from my last one (#24):
That should be all from me on here for a while, I don't think there are any more bugs :D
I'll have a Persona Essentials pr for you tomorrow as well then we should be good to release all these updates finally :)