feat(ovfs): add filesystem to handle message #4720
Conversation
zjregee
changed the title
zjregee
changed the title
| } | ||
|
|
||
| impl TryFrom<u32> for Opcode { | ||
| type Error = (); |
There was a problem hiding this comment.
Use () as error doesn't look good to me.
| /// InHeader represents the incoming message header in the filesystem call. | ||
| #[repr(C)] | ||
| #[derive(Debug, Default, Clone, Copy)] | ||
| pub struct InHeader { |
There was a problem hiding this comment.
This is a pub struct, how about adding comments for every field?
There was a problem hiding this comment.
I haven't found any official documentation describing what each field here means, and now each field here is just a reference to virtiofsd.
In addition, all structs here will only be used internally and will not be exposed to users. I'll add a link to virtiofsd, or I can add explanations for the fields I use.
There was a problem hiding this comment.
In addition, all structs here will only be used internally and will not be exposed to users.
Hi, I'd like comments here to make our lives easier. As time passes, we might forget what these fields mean. Leaving comments can save readers a lot of time by reducing guesswork.
I'll add a link to virtiofsd, or I can add explanations for the fields I use.
Adding a link to virtiofsd is enough. And adding explanations for used fields will be very helpful.
| out: Option<T>, | ||
| data: Option<&[u8]>, | ||
| unique: u64, | ||
| mut w: Writer, |
There was a problem hiding this comment.
How about make reply_ok a function of Writer? Does this idea make sense?
There was a problem hiding this comment.
It seems unnecessary. If I add a function called write_objs to the writer, I still need to encapsulate a reply_ok in the filesystem. If I move reply_ok directly to the writer, it will introduce certain semantics that the writer does not need to be aware of.
I now put both methods reply_ok reply_error inside the filesystem.
There was a problem hiding this comment.
Ok, we can refactor this part while ovfs is ready to use.
|
Hi, @Xuanwo, it's ready for review now. |
No description provided.