obhq / obliteration

Experimental PS4 emulator written in Rust for Windows, macOS and Linux
https://obliteration.net
MIT License
601 stars 17 forks source link

Find a solution to remove d_fdopen #730

Closed ultimaweapon closed 4 months ago

SuchAFuriousDeath commented 4 months ago

What about: Having a single function 'open' on the Device trait that returns an Arc? We can pass it an Option<&mut VFile> and every implementing type of the trait can match on it on its own and do the corresponding action.

SuchAFuriousDeath commented 4 months ago

This whole situation kind of reminds me of nameidata, by the way. You compromised on that, because it was too complicated, so you made it simpler. We lost some functionality, but it's not a big deal, we can just implement it, when we actually need it. Why not do the same where? Besides, why is open even implemented for devfs? We could've left a todo! and implemented it when we reached it.

ultimaweapon commented 4 months ago

@SuchAFuriousDeath can you start by explaining the problem you have faced?

SuchAFuriousDeath commented 4 months ago

This question kind of confuses me :D I don't know how to solve this issue, because we haven't even seen encountered a single device that actually uses fdopen. I don't even know what a 'typical' fdopen implementation looks like and what it does. How am I supposed to come up with a solution to replace it? It feels like the proper solution is to remove the Cdevsw struct , put a todo! in the dev::VnodeBackend::open function and wait until we actually start encountering devices. Then, we can slowly start building up code. I also think that if don't want dyn Any everywhere, the Device trait should have an 'open' function returning a Arc\<Self> (or maybe a Box\<Self> like I mentioned above. This Self shall hold any driver-specific fields, instead of d_open setting them in the cdev structure like FreeBSD does it. And if we ever reach a device that has f_fdopen instead of open, we'll cross that bridge when we get there. That's my suggestion.

ultimaweapon commented 4 months ago

Why do we need to remove the VnodeBackend::open that already implemented?

SuchAFuriousDeath commented 4 months ago

Well, because in it's current state, it requires fdopen :D But web haven't encountered an example yet, So we don't know how exactly to replace it with the trait based approach

SuchAFuriousDeath commented 4 months ago

This whole conversation is really confusing. I thought that you said that you want charakter devices to be more Rusty too. Why do we worry about fdopen when we don't even know if we need it?

ultimaweapon commented 4 months ago

Well, because in it's current state, it requires fdopen :D But web haven't encountered an example yet, So we don't know how exactly to replace it with the trait based approach

Why we don't remove d_fdopen instead of VnodeBackend::open? I just checked and no any devices implemented it.

SuchAFuriousDeath commented 4 months ago

That's kind of close to what I suggested. I suppose that we don't have to remove the function entirely. yeah