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.

Sign up for GitHub

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

Jump to bottom

Fix constants fs #98

Closed
wants to merge 4 commits into from
Closed

Fix constants fs #98

wants to merge 4 commits into from

Conversation

ChinmayShringi
Copy link
Contributor

@ChinmayShringi ChinmayShringi commented Nov 23, 2024 •
edited
Loading

Description

Fixes - Review and document constants in Lind-WASM
(Lind-Project/lind-wasm#46)
(Lind-Project/lind-wasm#30)

This PR reviews and updates the constants defined in the following files:

  • src/constants/mod.rs
  • src/constants/fs_constants.rs
  • src/constants/net_constants.rs
  • src/constants/sys_constants.rs
  • src/constants/vmmap_constants.rs

Key changes:

  1. Added comprehensive source references to Linux kernel v6.5 headers
  2. Added detailed documentation for each constant group
  3. Added explanatory comments for individual constants
  4. Identified platform-specific and Lind-specific constants
  5. Fixed type inconsistencies (u32/i32) where needed
  6. Organized constants into logical groups
  7. Added references to POSIX.1-2017 standard

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • cargo build

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have verified all constants against official Linux kernel sources
  • I have documented platform-specific considerations
  • I have identified and documented Lind-specific constants
  • I have cross-referenced constants with POSIX standards where applicable

@ChinmayShringi ChinmayShringi changed the base branch from main to vmmap-alice November 23, 2024 22:41
Yaxuan-w
Yaxuan-w reviewed Nov 23, 2024
View reviewed changes
Copy link
Member

@Yaxuan-w Yaxuan-w left a comment

Choose a reason for hiding this comment

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

Awesome! Minor suggestions

src/constants/mod.rs Outdated

// ===== Memory Protection Flags =====
// Source: include/uapi/asm-generic/mman-common.h
// These flags control memory segment permissions for mmap and mprotect
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to move those to vmmap_constant.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed them from mod.rs

src/constants/mod.rs Outdated
pub const O_TRUNC: i32 = 0o1000; // Truncate file to zero length
pub const O_APPEND: i32 = 0o2000; // Append mode - writes always at end

// ===== File Types =====
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused why we keep those here instead of moving to corresponding files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed them from mod.rs

src/safeposix/cage.rs Outdated

// Remove these old imports since we're using the central constants module now
// pub use super::syscalls::fs_constants::*;
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to delete directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed commtented code

src/safeposix/syscalls/mod.rs Outdated
pub use sys_calls::*;
pub use sys_constants::*;

// Removed constants after centralisation
Copy link
Member

Choose a reason for hiding this comment

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

Same as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed commtented code

rennergade
rennergade reviewed Nov 24, 2024
View reviewed changes
src/constants/vmmap_constants.rs
@@ -0,0 +1,55 @@
//! Virtual Memory Mapping Constants Module
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should get rid of this file and move all of this to fs_constants

@rennergade
Copy link
Contributor

This is great, love the way you set this up. Can you make the change to combine vmmap constants into fs constants. Then I'd like you to set up this PR to be into the monorepo since we're going to deprecate this repository shortly.

@ChinmayShringi
Copy link
Contributor Author

Closing this PR since changes were ported on the monorepo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rennergade rennergade rennergade left review comments

@yzhang71 yzhang71 Awaiting requested review from yzhang71

@Yaxuan-w Yaxuan-w Awaiting requested review from Yaxuan-w

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChinmayShringi @rennergade @Yaxuan-w