Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move fd table to crates and update rust-fatfs #99

Merged
merged 1 commit into from
Jun 1, 2024

Conversation

lhw2002426
Copy link
Contributor

No description provided.

@coolyjg coolyjg self-requested a review April 23, 2024 06:25
Copy link
Contributor

@coolyjg coolyjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put ruxfdtable into modules instead of crates.

///Rust version for struct timespec in ctypes. Represents a high-resolution time specification.
pub struct rux_timespec {
/// Whole seconds part of the timespec.
pub tv_sec: ::core::ffi::c_longlong,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use core::ffi::c_longlong


///Rust version for struct stat in ctypes. Represents file status information.
#[cfg(target_arch = "aarch64")]
pub struct Ruxstat {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use RuxStat. Structure names should follow Rust rules(Upper camel case).

crates/ruxfdtable/src/lib.rs Outdated Show resolved Hide resolved
modules/ruxfs/Cargo.toml Outdated Show resolved Hide resolved
@@ -202,8 +204,20 @@ impl AxRunQueue {
}
}

fn gc_flush_file(fd: usize) -> LinuxResult {
info!("lhw debug gc flush");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this or change into trace!("gc task flush: {}", fd)

modules/ruxtask/src/run_queue.rs Outdated Show resolved Hide resolved
fn gc_entry() {
let mut now_file_fd: usize = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to ignore stdin, stdout, stderr. (Start from 3)

use spin::RwLock;

///Rust version for struct timespec in ctypes. Represents a high-resolution time specification.
pub struct rux_timespec {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow upper camel case: RuxTimeSpec

FD_TABLE.write().add_at(0, Arc::new(stdin()) as _).unwrap(); // stdin
FD_TABLE.write().add_at(1, Arc::new(stdout()) as _).unwrap(); // stdout
FD_TABLE.write().add_at(2, Arc::new(stdout()) as _).unwrap(); // stderr
0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this initialization take effect? This should be put into another place (Like ruxruntime or somewhere).


///Rust version for struct stat in ctypes. Represents file status information.
#[cfg(target_arch = "aarch64")]
pub struct RuxStat {
Copy link
Contributor

@ken4647 ken4647 May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question about the struct RuxStat: why this variable need different versions for different arch?

Copy link
Contributor

@ken4647 ken4647 May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is possible to use #[cfg(...)] inside the struct rather than two standalone defination if a little difference exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the fields, like st_nlink's type is different, and some fields' order is different. So here two different structs are maintained. But it is possible to make it concise: use #[cfg(...)] for st_nlink and __unused, re-order the fields when transferring to C struct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the fields, like st_nlink's type is different, and some fields' order is different. So here two different structs are maintained. But it is possible to make it concise: use #[cfg(...)] for st_nlink and __unused, re-order the fields when transferring to C struct.

Thanks, I understand now. If the difference is the order of field, using #[cfg(...)] might make code messy. I think it will be okay to keep the code as it is now.

Copy link
Contributor

@coolyjg coolyjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about my previous misunderstanding. lazy_static will only be execute once.

api/ruxos_posix_api/src/imp/fd_ops.rs Show resolved Hide resolved
edition = "2021"
authors = ["HaoWen Liu <lhw2002426@stu.pku.edu.cn>"]
description = "fd table for ruxos file system"
license = "GPL-3.0-or-later OR Apache-2.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change License to MULAN

lazy_static::lazy_static! {
/// Global file descriptor table protected by a read-write lock.
pub static ref FD_TABLE: RwLock<FlattenObjects<Arc<dyn FileLike>, RUX_FILE_LIMIT>> = {
let mut fd_table = FlattenObjects::new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fd_table does not need to be mutable here.

@@ -197,7 +208,8 @@ pub fn sys_open(filename: *const c_char, flags: c_int, mode: ctypes::mode_t) ->
syscall_body!(sys_open, {
let options = flags_to_options(flags, mode);
let file = ruxfs::fops::File::open(filename?, &options)?;
File::new(file).add_to_fd_table()
let res = File::new(file).add_to_fd_table();
res
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary let binding

@@ -227,7 +239,8 @@ pub fn sys_openat(fd: usize, path: *const c_char, flags: c_int, mode: ctypes::mo
.lock()
.open_file_at(path?, &options)?
};
File::new(file).add_to_fd_table()
let res = File::new(file).add_to_fd_table();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary let binding

@ken4647 ken4647 merged commit 28131d9 into syswonder:dev Jun 1, 2024
10 checks passed
@coolyjg coolyjg mentioned this pull request Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants