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

General Repository Cleanup #480

Merged
merged 18 commits into from
May 21, 2024
Merged

General Repository Cleanup #480

merged 18 commits into from
May 21, 2024

Conversation

FelixMcFelix
Copy link
Collaborator

@FelixMcFelix FelixMcFelix commented Mar 28, 2024

This PR brings some code cleanliness changes and a bugfix:

  • Make use of the new addr_of_mut!()/addr_of!() macros where shared mutable statics are used as part of MAC registration.
  • Convert CStrs into c"" syntax post-rust 1.77.0 (for non-bindgen code, at least).
  • Fix up clippy lints which have piled up, most recently the 'redundant import' checks.
  • Fix an issue where all TCP packets + sizes would be recorded on the 'inbound' stats.
  • Prevent derror_macro from being pulled into Omicron's lockfile.

I spent a little time looking into the non-Send+Sync in Arc clippy lints, but those will require a separate PR to assess the safety of impl Send+Sync on, e.g., KStatNamed.

Since the recent refactor, count + bytes have all been ending up in
`segs_in`/`bytes_in`, which can make for some confusing reading at the
best of times.
Unfortunately the Arc around non Send+Sync will need some fairly
intrusive bound insertion, by the looks of things. When we do that it
should be its own PR.
@FelixMcFelix FelixMcFelix marked this pull request as ready for review March 28, 2024 18:16
@FelixMcFelix FelixMcFelix self-assigned this Apr 22, 2024
Comment on lines +1598 to +1607
match *dir {
TcpDirection::In { .. } => {
tfes.segs_in += 1;
tfes.bytes_in += pkt_len;
}
TcpDirection::Out { .. } => {
tfes.segs_out += 1;
tfes.bytes_out += pkt_len;
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bugfix for TCP statistics visible from opteadm dump-tcp-flows.

@@ -8,7 +8,6 @@
#![feature(extern_types)]
#![feature(panic_info_message)]
#![no_std]
#![allow(non_camel_case_types)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Redundant allow wrt. below block dedicated to ip.rs/bindgen.

@@ -307,13 +308,13 @@ struct XdeDev {
#[no_mangle]
unsafe extern "C" fn _init() -> c_int {
xde_devs.init(KRwLockType::Driver);
mac::mac_init_ops(&mut xde_devops, XDE_STR);
mac::mac_init_ops(addr_of_mut!(xde_devops), XDE_STR);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This (and any other addr_of/addr_of_mut) is a UB fix we've been warned about for a month or two, but will become mandatory in the next edition.

@@ -2,7 +2,7 @@
"arch": "x86_64",
"code-model": "kernel",
"cpu": "x86-64",
"data-layout": "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128",
"data-layout": "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mandated by latest nightly compilers after a related change in rust 1.77.

@@ -3,7 +3,7 @@
#: name = "opte-api"
#: variety = "basic"
#: target = "helios-2.0"
#: rust_toolchain = "nightly-2024-02-06"
#: rust_toolchain = "nightly-2024-04-25"
Copy link

Choose a reason for hiding this comment

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

Dumb question, but why doesn't rust-toolchain.toml work here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is down to us needing several nightly format options from cargo fmt across the repo. However, everything apart from XDE compiles on a stable toolchain version (as XDE is reliant on several unstable feature flags).

lib/opte/src/d_error.rs Outdated Show resolved Hide resolved
@FelixMcFelix FelixMcFelix added this to the 9 milestone May 13, 2024
@FelixMcFelix FelixMcFelix requested a review from mkeeter May 14, 2024 23:19
@FelixMcFelix FelixMcFelix merged commit 6a6abb2 into master May 21, 2024
10 checks passed
@FelixMcFelix FelixMcFelix deleted the spring-clean branch May 21, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants