Reconsider file backend implementation.

[SuchAFuriousDeath]

So, we already talked about this. I'm now testing AC4 after implementing sys_read. The game creates a socket and tries to call connect on it. I need to implement Filedesc::get_socket for that, Since our file backend is a Box<dyn FileBackend>, it's famously very inconvenient to downcast it to a specific implementation. By using dyn, we also have to create a wrapper struct for every file backend to implement the FileBackend trait on. The alternative I suggest would be returning to the old way this was done and simply correcting my previous oversight by having something like

enum VFileType {
Vnode(VnodeFileType)
...
}

enum VnodeFileType {
Vnode(Arc<Vnode>),
Device(Arc<CharacterDevice>)
}

This would disallow inconsistent state (that we technically allow now) while staying true to FreeBSD.

As a sidenote, I also don't like the is_seekable + vnode field combo. This. once again allows nonsense state like a vnode file backend without a vnode, something that cannot happen. I don't really see the point in allowing this. I liked the seekable_vnode method better. It also allows us to reuse the existing stored vnode in some of the file backends instead of having it as a separate field.

Let me know your thoughts.

[SuchAFuriousDeath]

I also think this is clearer to any future developers. They might be confused that devices have the type of DTYPE_VNODE even though there's DTYPE_DEV.

[ultimaweapon]

The problems with your solution are:

1. There will be a lot of enum variants and every time we need to forward a call to backend we need to do it on every variant.
2. It will pollute the file module with every file in the system because it needs to import the backend type to put in the enum variants.

[SuchAFuriousDeath]

I'm aware of this and still consider it better than dyn 😅. I don't really see 2) as a big problem. Is it really that bad to have ~6-10 imports instead of having to create a separate type for every would-be variant (or perhaps I miseunderstood something)? And tbh 1) is also not really a problem for me. it was already done 'my way' and the world didn't end. Maybe we would've added a couple more enum variants over time. Not a big deal to me, especially when the alternative is using dyn, allowing nonsense-state, having several unwraps in places that don't really make sense and having to create extra wrapper structs.

[ultimaweapon]

It is a problem because it makes FS depend on unnecessary things. The unwrap problem can be solved without your solution.

[SuchAFuriousDeath]

I admit I still don't really see why that's bad.

[ultimaweapon]

One of the rules for good software architecture is modular. It is the same as: Why serde does not include all file formats within it own? Why it provide only a foundation and split each format into a separated crate?

[SuchAFuriousDeath]

That I understand (and agree with). I just don't see how that applies to this situation. The filesystem IS related to... things that can be a file 😅. It seems to me that calling it unnecesarry is circular logic. Right now, with dyn, it is unnecesarry, but if we accept my arguments, then it is not unnecesarry.
Previously, our vnode item field was a Box<dyn Any>. I changed that to be a new type 'VnodeItem', which is an enum. I'm not sure if you approved that or someone else did, but take a look at it now. Do you think it was a bad choice? With Box<dyn Any> you can put literally anything there, even things that are not supposed to be there. I would argue that it's better to have an enum, since it gives someone trying to understand the code a clear idea of what items a vnode can have. But with Box<dyn Any>, it can be literally anything and you have to go through all the instances of when a vnode item is created to have full knowledge of what's going on.
Similarly, if we did things my way, someone reading the code would have a clear idea of what file types go together with what file backends and literally couldn't instantiate a wrong combination of those. With your approach, there are secret, slightly different dependencies - For example, let's say that we implement the get_socket method by checking the file type and then downcasting the backend to a socket. The correctness of this implementation now relies on the fact that every piece of code that creates a socket has to pass the right file type. But that's not a guarantee, since they are two independent fields. A hidden dependency is created this way and it's completely implicit and disconnected from the get_socket method. Similarly, you can currently have a vnode file type with the f_vnode being None.
In conclusion: I agree that my approach has downsides, however, I think they are relatively minor compared to what you suggest (and what we currently have). If we accept what I claim here, then I don't think the dependency is 'unnecesarry'.

[ultimaweapon]

The filesystem IS related to... things that can be a file 😅.

The FS itself should be a foundation to implement filesystem, not contains all of FS within it own (e.g. kqueue that depend on FS but the FS itself know nothing about the kqueue). The meaning of this is every FS relating things should be able to detach itself from the FS without the need to modify the FS itself. This is also how FS in the FreeBSD is implemented.

Previously, our vnode item field was a Box. I changed that to be a new type 'VnodeItem', which is an enum. I'm not sure if you approved that or someone else did, but take a look at it now. Do you think it was a bad choice? With Box you can put literally anything there, even things that are not supposed to be there. I would argue that it's better to have an enum, since it gives someone trying to understand the code a clear idea of what items a vnode can have. But with Box, it can be literally anything and you have to go through all the instances of when a vnode item is created to have full knowledge of what's going on.

It is okay as long as the items is anything inside the FS itself, but not okay if it is going to pull other FS implementations from the outside.

Similarly, if we did things my way, someone reading the code would have a clear idea of what file types go together with what file backends and literally couldn't instantiate a wrong combination of those. With your approach, there are secret, slightly different dependencies - For example, let's say that we implement the get_socket method by checking the file type and then downcasting the backend to a socket. The correctness of this implementation now relies on the fact that every piece of code that creates a socket has to pass the right file type. But that's not a guarantee, since they are two independent fields. A hidden dependency is created this way and it's completely implicit and disconnected from the get_socket method. Similarly, you can currently have a vnode file type with the f_vnode being None.

Then why we don't find a correct solution for this? If you think your solution is the only answer here but I don't think so.

In conclusion: I agree that my approach has downsides, however, I think they are relatively minor compared to what you suggest (and what we currently have). If we accept what I claim here, then I don't think the dependency is 'unnecesarry'.

I don't even suggest any solution to fix the problem yet. All I said is: The unwrap problem can be solved without your solution.

[SuchAFuriousDeath]

i'm open to other solutions.

[ultimaweapon]

I created an issue for this: #848