Skip to content

Memory manager clarity improvements#987

Merged
sporksmith merged 10 commits intoshadow:devfrom
sporksmith:memory-manager-logging
Sep 17, 2020
Merged

Memory manager clarity improvements#987
sporksmith merged 10 commits intoshadow:devfrom
sporksmith:memory-manager-logging

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Sep 15, 2020 •
edited
Loading

  • Switch from println! to logging macros.
  • Use nix function wrappers instead of libc wrappers.
  • Use nix Result and Error types, which use an Errno type, rather than just an i32 to represent errno errors.
  • Simplify some pointer arithmetic using std::ptr::add, and util::ops::Range::len.
  • Flesh out some comments.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 15, 2020 •
edited
Loading

Codecov Report

Merging #987 into dev will increase coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #987      +/-   ##
==========================================
+ Coverage   52.95%   53.27%   +0.32%     
==========================================
  Files         126      126              
  Lines       18187    18308     +121     
  Branches     4329     4385      +56     
==========================================
+ Hits         9631     9754     +123     
+ Misses       5861     5852       -9     
- Partials     2695     2702       +7
Flag Coverage Δ
#tests 53.27% <ø> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/shmem/shmem_file.c 57.54% <0.00%> (+6.60%) ⬆️
src/main/shmem/shmem_cleanup.c 63.46% <0.00%> (+9.61%) ⬆️
src/support/logger/rust_bindings/src/lib.rs 30.28% <0.00%> (+14.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b0ef1...167034f. Read the comment docs.

@sporksmith sporksmith mentioned this pull request Sep 16, 2020
Merged
@sporksmith sporksmith force-pushed the memory-manager-logging branch from ef733f1 to b8a2292 Compare September 17, 2020 00:29
@sporksmith sporksmith marked this pull request as ready for review September 17, 2020 01:14
@sporksmith sporksmith changed the title Memory manager logging Memory manager clarity improvements Sep 17, 2020
stevenengler
stevenengler reviewed Sep 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

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

Looks good!

src/main/host/memory_manager.rs Outdated
Comment on lines +853 to +854
/* old_len: */ self.heap.end - self.heap.start,
/* new_len: */ new_heap.end - new_heap.start,
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler Sep 17, 2020

Choose a reason for hiding this comment

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

Can these use range.len()?

Copy link
Copy Markdown
Contributor Author

@sporksmith sporksmith Sep 17, 2020

Choose a reason for hiding this comment

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

Yup, missed these ones. Thanks!

src/main/host/memory_manager.rs Outdated
Comment on lines +1005 to +1010
// Extend the portion of the stack that we've mapped downward to include `src`.
//
// This is carefuly designed *not* to invalidate any outstanding borrowed references or
// pointers, since otherwise a caller trying to marshall multiple syscall arguments might
// invalidate the first argument when marshalling the second. (While the Rust API currently
// prevents there from being any outstanding references, the C API does not).
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler Sep 17, 2020

Choose a reason for hiding this comment

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

Make these /// so that they show up in the docs? (And for other functions in this file.)

Copy link
Copy Markdown
Contributor Author

@sporksmith sporksmith Sep 17, 2020

Choose a reason for hiding this comment

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

Does it make sense to use /// for non-public methods? I suppose it doesn't hurt...

Copy link
Copy Markdown
Contributor

@stevenengler stevenengler Sep 17, 2020

Choose a reason for hiding this comment

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

I think it makes sense, since if you're developing the library itself and not just using it in another project, you may run cargo doc --document-private-items which will include those functions. And rust seems to use /// for non-public methods as well: https://doc.rust-lang.org/src/alloc/vec.rs.html#2978

Copy link
Copy Markdown
Contributor Author

@sporksmith sporksmith Sep 17, 2020

Choose a reason for hiding this comment

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

SG. Thanks!

robgjansen
robgjansen reviewed Sep 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

Thanks for the extra comments! I'm happy if Steven's happy :)

@robgjansen robgjansen added Type: Maintenance Refactoring, cleanup, documenation, or process improvements Component: Main Composing the core Shadow executable labels Sep 17, 2020
@stevenengler
Copy link
Copy Markdown
Contributor

stevenengler commented Sep 17, 2020

Thanks for the extra comments! I'm happy if Steven's happy :)

I'm always happy so that's not a good basis for decision making :)

(I thought I approved this PR when I left a review, but I guess not, so approving now.)

@sporksmith sporksmith merged commit adce258 into shadow:dev Sep 17, 2020
@sporksmith sporksmith deleted the memory-manager-logging branch September 17, 2020 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robgjansen robgjansen robgjansen left review comments

@stevenengler stevenengler stevenengler approved these changes

Assignees

@sporksmith sporksmith

Labels

Component: Main Composing the core Shadow executable Type: Maintenance Refactoring, cleanup, documenation, or process improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sporksmith @stevenengler @robgjansen