* [PATCH] rust: Add functions to create and modify an Inode @ 2023-04-13 14:01 Ariel Miculas 2023-04-13 18:36 ` Miguel Ojeda 0 siblings, 1 reply; 6+ messages in thread From: Ariel Miculas @ 2023-04-13 14:01 UTC (permalink / raw) To: rust-for-linux; +Cc: Ariel Miculas Add try_new_inode function associated to SuperBlock which allocates a new inode and implement a few setter functions for INode. Signed-off-by: Ariel Miculas <amiculas@cisco.com> --- rust/kernel/fs.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs index 1ba01629e9cd..58c281123c28 100644 --- a/rust/kernel/fs.rs +++ b/rust/kernel/fs.rs @@ -707,6 +707,18 @@ pub struct SuperBlock<T: Type + ?Sized>( PhantomData<T>, ); +impl<T: Type + ?Sized> SuperBlock<T> { + /// Create a new inode + pub fn try_new_inode(&self) -> Result<INode> { + let inode = unsafe { bindings::new_inode(self.0.get()) }; + if inode.is_null() { + return Err(ENOMEM); + } + // SAFETY: We've just allocated a new inode and checked that it's not null + Ok(INode(UnsafeCell::new(unsafe { *inode }))) + } +} + /// Wraps the kernel's `struct inode`. /// /// # Invariants @@ -716,6 +728,35 @@ pub struct SuperBlock<T: Type + ?Sized>( #[repr(transparent)] pub struct INode(pub(crate) UnsafeCell<bindings::inode>); +impl INode { + /// set the field i_mode of the inode + pub fn set_mode(&mut self, mode: u16) { + self.0.get_mut().i_mode = mode; + } + /// set the field i_ino of the inode + pub fn set_ino(&mut self, ino: u64) { + self.0.get_mut().i_ino = ino; + } + + /// set the current time in the atime, ctime and mtime fields of the inode + pub fn set_current_time(&mut self) { + let time = unsafe { bindings::current_time(self.0.get()) }; + self.0.get_mut().i_atime = time; + self.0.get_mut().i_ctime = time; + self.0.get_mut().i_mtime = time; + } + + /// set the field i_fop of the inode + pub fn set_fop(&mut self, fop: *const bindings::file_operations) { + self.0.get_mut().__bindgen_anon_3.i_fop = fop; + } + + /// set the field i_op of the inode + pub fn set_op(&mut self, fop: *const bindings::inode_operations) { + self.0.get_mut().i_op = fop; + } +} + // SAFETY: The type invariants guarantee that `INode` is always ref-counted. unsafe impl AlwaysRefCounted for INode { fn inc_ref(&self) { -- 2.40.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: Add functions to create and modify an Inode 2023-04-13 14:01 [PATCH] rust: Add functions to create and modify an Inode Ariel Miculas @ 2023-04-13 18:36 ` Miguel Ojeda 2023-04-13 18:59 ` Ariel Miculas [not found] ` <CAPDJoNt_jCPfkmS7pRVA=1-U6csPEMuYsjTdwYm3HpUyZBVEUQ@mail.gmail.com> 0 siblings, 2 replies; 6+ messages in thread From: Miguel Ojeda @ 2023-04-13 18:36 UTC (permalink / raw) To: Ariel Miculas; +Cc: rust-for-linux, Ariel Miculas Hi Ariel, On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote: > > Add try_new_inode function associated to SuperBlock which allocates a > new inode and implement a few setter functions for INode. Thanks for the patch! This seems to be based on the `rust` branch: the filesystem abstractions we have there are not yet in mainline, so we can't take this now since `fs.rs` does not exist yet. Please see https://rust-for-linux.com/branches. Also, I haven't reviewed the patch, but some quick notes below that you may find useful. > + let inode = unsafe { bindings::new_inode(self.0.get()) }; Please follow the coding conventions / guidelines of the rest of other code. For instance, we require `// SAFETY` comments on all `unsafe` blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html for some details. > + /// set the field i_mode of the inode Ditto, for docs. > + pub fn set_fop(&mut self, fop: *const bindings::file_operations) { If this is intended to be used by modules (as it appears to be since it is public in a public module), then you will likely need to avoid referring to `bindings::` so that modules do not need to deal directly with the C types. Moreover, the fact that a safe function takes a raw pointer raises some questions -- what happens if somebody passes a dangling pointer, for instance? Cheers, Miguel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: Add functions to create and modify an Inode 2023-04-13 18:36 ` Miguel Ojeda @ 2023-04-13 18:59 ` Ariel Miculas 2023-04-13 19:40 ` Wedson Almeida Filho [not found] ` <CAPDJoNt_jCPfkmS7pRVA=1-U6csPEMuYsjTdwYm3HpUyZBVEUQ@mail.gmail.com> 1 sibling, 1 reply; 6+ messages in thread From: Ariel Miculas @ 2023-04-13 18:59 UTC (permalink / raw) To: Miguel Ojeda; +Cc: rust-for-linux, Ariel Miculas Thanks Miguel, you've clarified some questions I had about the branches and the development process. Regarding the rust branch, it says: > However, please feel free to submit new PRs for this branch. Does it make sense to open PRs against the rust branch in github and ask for feedback that way or should the existing fs.rs from the rust branch be merged into rust-next instead (and then I could submit patches)? For context, I'm trying to add the necessary fs abstractions in order to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust. I want to ask for feedback early because I'm not sure the abstractions I come up with are the right ones. Regards, Ariel On Thu, Apr 13, 2023 at 9:36 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Hi Ariel, > > On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote: > > > > Add try_new_inode function associated to SuperBlock which allocates a > > new inode and implement a few setter functions for INode. > > Thanks for the patch! > > This seems to be based on the `rust` branch: the filesystem > abstractions we have there are not yet in mainline, so we can't take > this now since `fs.rs` does not exist yet. Please see > https://rust-for-linux.com/branches. > > Also, I haven't reviewed the patch, but some quick notes below that > you may find useful. > > > + let inode = unsafe { bindings::new_inode(self.0.get()) }; > > Please follow the coding conventions / guidelines of the rest of other > code. For instance, we require `// SAFETY` comments on all `unsafe` > blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html > for some details. > > > + /// set the field i_mode of the inode > > Ditto, for docs. > > > + pub fn set_fop(&mut self, fop: *const bindings::file_operations) { > > If this is intended to be used by modules (as it appears to be since > it is public in a public module), then you will likely need to avoid > referring to `bindings::` so that modules do not need to deal directly > with the C types. > > Moreover, the fact that a safe function takes a raw pointer raises > some questions -- what happens if somebody passes a dangling pointer, > for instance? > > Cheers, > Miguel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: Add functions to create and modify an Inode 2023-04-13 18:59 ` Ariel Miculas @ 2023-04-13 19:40 ` Wedson Almeida Filho 2023-04-13 19:59 ` Ariel Miculas 0 siblings, 1 reply; 6+ messages in thread From: Wedson Almeida Filho @ 2023-04-13 19:40 UTC (permalink / raw) To: Ariel Miculas; +Cc: Miguel Ojeda, rust-for-linux, Ariel Miculas On Thu, 13 Apr 2023 at 16:05, Ariel Miculas <ariel.miculas@gmail.com> wrote: > > Thanks Miguel, you've clarified some questions I had about the > branches and the development process. > Regarding the rust branch, it says: > > However, please feel free to submit new PRs for this branch. > > Does it make sense to open PRs against the rust branch in github and > ask for feedback that way or should the existing fs.rs from the rust > branch be merged into rust-next instead (and then I could submit > patches)? > For context, I'm trying to add the necessary fs abstractions in order > to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust. > I want to ask for feedback early because I'm not sure the abstractions > I come up with are the right ones. Please checkout https://github.com/wedsonaf/linux/commits/fs This is the continuation of the code currently in the `rust` branch. It allows the creation of a read-only in-memory file system as you can see in the sample code. I was working on it with the intent of merging it to `rust` but then we decided to cut down what was upstreamed to a bare minimum, so I put this on the back burner while we push other pieces (on which this depends) upstream first. We also heard from Matthew Wilcox (the page cache maintainer) on LPC that he has ideas on what the interface should (or shouldn't) look like. So we'll get him involved [if he has the time] once we have the dependencies upstreamed. Some of the code we know will change for the better now that we have pin-init. Anyway, you're welcome to join us if you're interested. Cheers, -Wedson > Regards, > Ariel > > > On Thu, Apr 13, 2023 at 9:36 PM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > Hi Ariel, > > > > On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote: > > > > > > Add try_new_inode function associated to SuperBlock which allocates a > > > new inode and implement a few setter functions for INode. > > > > Thanks for the patch! > > > > This seems to be based on the `rust` branch: the filesystem > > abstractions we have there are not yet in mainline, so we can't take > > this now since `fs.rs` does not exist yet. Please see > > https://rust-for-linux.com/branches. > > > > Also, I haven't reviewed the patch, but some quick notes below that > > you may find useful. > > > > > + let inode = unsafe { bindings::new_inode(self.0.get()) }; > > > > Please follow the coding conventions / guidelines of the rest of other > > code. For instance, we require `// SAFETY` comments on all `unsafe` > > blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html > > for some details. > > > > > + /// set the field i_mode of the inode > > > > Ditto, for docs. > > > > > + pub fn set_fop(&mut self, fop: *const bindings::file_operations) { > > > > If this is intended to be used by modules (as it appears to be since > > it is public in a public module), then you will likely need to avoid > > referring to `bindings::` so that modules do not need to deal directly > > with the C types. > > > > Moreover, the fact that a safe function takes a raw pointer raises > > some questions -- what happens if somebody passes a dangling pointer, > > for instance? > > > > Cheers, > > Miguel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: Add functions to create and modify an Inode 2023-04-13 19:40 ` Wedson Almeida Filho @ 2023-04-13 19:59 ` Ariel Miculas 0 siblings, 0 replies; 6+ messages in thread From: Ariel Miculas @ 2023-04-13 19:59 UTC (permalink / raw) To: Wedson Almeida Filho; +Cc: Miguel Ojeda, rust-for-linux, Ariel Miculas Thanks, Wedson, I wasn't aware of your branch. I'm definitely interested in helping out, let me know what I can do. I'm new to rust-for-linux, though, so don't expect too much. Regards, Ariel On Thu, Apr 13, 2023 at 10:41 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote: > > On Thu, 13 Apr 2023 at 16:05, Ariel Miculas <ariel.miculas@gmail.com> wrote: > > > > Thanks Miguel, you've clarified some questions I had about the > > branches and the development process. > > Regarding the rust branch, it says: > > > However, please feel free to submit new PRs for this branch. > > > > Does it make sense to open PRs against the rust branch in github and > > ask for feedback that way or should the existing fs.rs from the rust > > branch be merged into rust-next instead (and then I could submit > > patches)? > > For context, I'm trying to add the necessary fs abstractions in order > > to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust. > > I want to ask for feedback early because I'm not sure the abstractions > > I come up with are the right ones. > > Please checkout https://github.com/wedsonaf/linux/commits/fs > > This is the continuation of the code currently in the `rust` branch. > It allows the creation of a read-only in-memory file system as you can > see in the sample code. > > I was working on it with the intent of merging it to `rust` but then > we decided to cut down what was upstreamed to a bare minimum, so I put > this on the back burner while we push other pieces (on which this > depends) upstream first. We also heard from Matthew Wilcox (the page > cache maintainer) on LPC that he has ideas on what the interface > should (or shouldn't) look like. So we'll get him involved [if he has > the time] once we have the dependencies upstreamed. > > Some of the code we know will change for the better now that we have pin-init. > > Anyway, you're welcome to join us if you're interested. > > Cheers, > -Wedson > > > Regards, > > Ariel > > > > > > On Thu, Apr 13, 2023 at 9:36 PM Miguel Ojeda > > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > > > Hi Ariel, > > > > > > On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote: > > > > > > > > Add try_new_inode function associated to SuperBlock which allocates a > > > > new inode and implement a few setter functions for INode. > > > > > > Thanks for the patch! > > > > > > This seems to be based on the `rust` branch: the filesystem > > > abstractions we have there are not yet in mainline, so we can't take > > > this now since `fs.rs` does not exist yet. Please see > > > https://rust-for-linux.com/branches. > > > > > > Also, I haven't reviewed the patch, but some quick notes below that > > > you may find useful. > > > > > > > + let inode = unsafe { bindings::new_inode(self.0.get()) }; > > > > > > Please follow the coding conventions / guidelines of the rest of other > > > code. For instance, we require `// SAFETY` comments on all `unsafe` > > > blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html > > > for some details. > > > > > > > + /// set the field i_mode of the inode > > > > > > Ditto, for docs. > > > > > > > + pub fn set_fop(&mut self, fop: *const bindings::file_operations) { > > > > > > If this is intended to be used by modules (as it appears to be since > > > it is public in a public module), then you will likely need to avoid > > > referring to `bindings::` so that modules do not need to deal directly > > > with the C types. > > > > > > Moreover, the fact that a safe function takes a raw pointer raises > > > some questions -- what happens if somebody passes a dangling pointer, > > > for instance? > > > > > > Cheers, > > > Miguel ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CAPDJoNt_jCPfkmS7pRVA=1-U6csPEMuYsjTdwYm3HpUyZBVEUQ@mail.gmail.com>]
* Re: [PATCH] rust: Add functions to create and modify an Inode [not found] ` <CAPDJoNt_jCPfkmS7pRVA=1-U6csPEMuYsjTdwYm3HpUyZBVEUQ@mail.gmail.com> @ 2023-04-18 23:57 ` Miguel Ojeda 0 siblings, 0 replies; 6+ messages in thread From: Miguel Ojeda @ 2023-04-18 23:57 UTC (permalink / raw) To: Ariel Miculas; +Cc: rust-for-linux, Ariel Miculas On Thu, Apr 13, 2023 at 8:55 PM Ariel Miculas <ariel.miculas@gmail.com> wrote: > > Thanks Miguel, you've clarified some questions I had about the branches and the development process. My pleasure! > Regarding the rust branch, it says: > > However, please feel free to submit new PRs for this branch. > > Does it make sense to open PRs against the rust branch in github and ask for feedback that way or should the existing fs.rs from the rust branch be merged into rust-next instead (and then I could submit patches)? > For context, I'm trying to add the necessary fs abstractions in order to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust. I want to ask for feedback early because I'm not sure the abstractions I come up with are the right ones. Yes, in general, please feel free to submit PRs there. The PRs will not be merged, because the branch is effectively frozen and will eventually go away / get archived, but you may still find it useful to get early feedback. The mailing list is meant for mainline development instead. As for mainline, anyone can also send patches, of course, as long as they are ready to be applied (again, in general, e.g. for filesystem APIs, please wait until things get a bit more settled down as Wedson mentioned -- when in doubt, you can always ask in Zulip). Cheers, Miguel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-18 23:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-13 14:01 [PATCH] rust: Add functions to create and modify an Inode Ariel Miculas
2023-04-13 18:36 ` Miguel Ojeda
2023-04-13 18:59 ` Ariel Miculas
2023-04-13 19:40 ` Wedson Almeida Filho
2023-04-13 19:59 ` Ariel Miculas
[not found] ` <CAPDJoNt_jCPfkmS7pRVA=1-U6csPEMuYsjTdwYm3HpUyZBVEUQ@mail.gmail.com>
2023-04-18 23:57 ` Miguel Ojeda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox