dylanmc / cs393_vm_api Goto Github PK

I figure this is a good place to have a discussion about this API. Here's the current code, for reference:

1. Do we want to force the error type to be &str, or do we want to allow each implementer to provide their own error type? The latter is maybe slightly more complicated, but much, much more idiomatic.
2. Why does write need mutable access to the buffer? Very possible I'm misunderstanding, but my assumption would be it just needs to read out of the buffer so it can write that data into whatever its backing memory is.
3. Is there a reason we need to take a &mut Vec<u8> instead of &mut [u8]? My instinct is the latter is more general (since a &mut Vec can be downcast into a mutable slice, but not vice versa), but open to other considerations.

Happy to write a PR to resolve any/all of these, but wanted to raise them first, since they're more fluid design choices then my other PRs.

Putting on my "code pedant" hat:

1. DataSource calls them maps in its APIs (e.x. DataSource::add_map), but AddressSpace calls them mappings (e.x. AddressSpace::add_mapping).
2. DataSource uses del, but AddressSpace uses remove for their respective remove_map APIs.

I don't think we need dynamic dispatch for these methods, since they're only dealing with one DataSource at once. I think only the underlying storage of mappings needs to dynamically dispatch. Using generics in these methods will allow monomorphization, which is likely faster.

This is the Big Question that we talked about a lot in class; I just want a centralized place for this discussion. Some initial thoughts:

1. We probably want our own linked list, if we're using linked lists. Probably regardless we need our own implementation, both for no_std purposes and to allow the hack of writing pointers into unallocated pages.
2. If we want to avoid dynamic dispatch, we need to avoid having a big collection of different kinds of DataSources. I'm not sure that's possible in a monolithic kernel, unless we want applications to deal with multiple AddressSpaces, one for each DataSource they rely on. That seems painful.
3. VecDeque (which is a circular buffer, i.e. the kind of datastructure used in log-structured file systems) might be better than LinkedList. Certainly it's a more rust-y data structure, and I think the standard library's implementation is generally considered to be much more efficient. There are possibly other data structures we could find in the ecosystem or roll ourselves that would be even more suited to the task.

The methods of AddressSpace take &dyn DataSource, e.g.:

But the struct itself stores Arc<dyn DataSource>:

My instinct is that add_mapping should just take an Arc. The broader issue is that the methods of DataSource only have shared access to self, and there's no way to safely upcast &self to Arc<self>. So as I see it, either:

1. The DataSource methods to have a stronger reference to self.
2. MapType needs to store &dyn DataSource (maybe fine! The DataSource API is immutable anyway).
3. There's some clever invariant that will make it ok to unsafely mutate &dyn DataSource into Arc<dyn DataSource>. This seems pretty unlikely to me.

This is weird. The word "sonatype" does not appear in the repo, certainly not in the .github folder. It might be enabled somewhere in the repository settings?

Want to raise this as a discussion issue rather than just writing the PR, but I think that remove_mapping should just take a MapEntry as argument. In particular, this will let us use the borrow checker to check lifetimes of mappings, since remove_mapping will drop the MapEntry, so we can guarantee that the user no longer has a reference to it after it's removed. (On a broader note, this is why I don't like the idea of "fighting the borrow-checker": this is a case where we can make the borrow-checker work for us to ensure correctness, not just to ensure memory safety.)