rawposix - misc fixes #341
Conversation
| @@ -407,6 +407,15 @@ impl Cage { | |||
| return syscall_error(Errno::EINVAL, "sigkill", "Invalid signal number"); | |||
| } | |||
|
|
|||
| // if cage id is 0, send signal to itself | |||
There was a problem hiding this comment.
can we expand this comment a bit?
I think from the manpage: If pid equals 0, then sig is sent to every process in the process
group of the calling process.
we should just explain that we dont have process groups so it just sends to itself if thats correct?
| @@ -311,13 +311,13 @@ impl Cage { | |||
| // reach here means we already found the desired exited child | |||
| let zombie = zombie_opt.unwrap(); | |||
| // update the status | |||
| *status = zombie.exit_code; | |||
| let _ = status.map(|st| *st = zombie.exit_code); | |||
There was a problem hiding this comment.
im probably just too much of a newbie at rust, but why do we use map here?
There was a problem hiding this comment.
This is equivalent to
if let Some(value) = status.as_mut() {
*value = zombie.exit_code;
}
It has no meaningful difference in terms of performance. Though this approach (map method) is indeed more difficult for new people to understand. Do we have a perference to write rust code with simple form for new people to understand?
There was a problem hiding this comment.
As a rule of thumb writing code that's simpler to follow is the best option, though adding a comment here also achieves the same results. I just wasn't sure if using map like this would be clear to anyone with basic understanding of rust. I'd either change it or add a comment.
| @@ -1435,9 +1435,10 @@ impl Cage { | |||
| return syscall_error(Errno::EBADF, "getdents", "Bad File Descriptor"); | |||
| } | |||
| let vfd = wrappedvfd.unwrap(); | |||
| // calling getdents64 instead of getdents since glibc expects result from getdents64 | |||
| let status = interface::get_i32_ref(status_addr).unwrap(); | ||
| // status could possibly be NULL | ||
| let status = { | ||
| if arg1 == 0 { |
There was a problem hiding this comment.
does dennis's pr affect this?
| @@ -633,6 +630,7 @@ pub fn brk_handler(cageid: u64, brk: u32) -> i32 { | |||
| } | |||
|
|
|||
| // update vmmap entry | |||
| vmmap.remove_entry(0, old_brk_page); | |||
There was a problem hiding this comment.
why do we have to call remove here?
There was a problem hiding this comment.
If I still remember correctly, add_entry_with_overwrite from vmmap does not automatically remove the old entries when the inserted new entries is overlapping with it.
There was a problem hiding this comment.
I'd comment that here then. Also worth checking that the comment for the function is clear about that wherever add_entry_with_overwrite lives
thank you for doing this. Can you update the description for this PR with bullet points of what the fixes are and what theyre needed for? |
rennergade
changed the title
This PR is splited from PR #164
This PR focuses on spliting out the some minor fixes on rawposix side
Detailed fix list:
add_entry_with_overwriteof vmmap, which does not automatically remove overlapping entries when inserting