[Closes #365] Fix TODOs in pipe.rs #362
Conversation
travis1829
requested review from
jeehoonkang
and removed request for
jeehoonkang
|
kernel-rs/src/pipe.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl AllocatedPipe { | ||
| pub fn alloc() -> Result<(RcFile<'static>, RcFile<'static>), ()> { | ||
| let page = kernel().alloc().ok_or(())?; | ||
| let ptr = page.into_usize() as *mut Pipe; | ||
| let mut ptr = NonNull::new(page.into_usize() as *mut Pipe).expect("AllocatedPipe alloc"); |
There was a problem hiding this comment.
Page의 invariant에 따라 page.into_usize() as *mut Pipe가 non-null이라는 보장이 있으므로 new_unchecked를 사용해서 run-time check를 줄이는 것도 괜찮아 보입니다.
There was a problem hiding this comment.
앞으로 NonNull 을 많이 사용하게 된다면 Page::into_non_null_ptr 같은 걸 만드는 것도 좋겠습니다. (https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.as_non_null_ptr)
| !inner.readopen && !inner.writeopen | ||
| } | ||
| } | ||
|
|
||
| /// # Safety | ||
| /// | ||
| /// `ptr` always refers to a `Pipe`. |
There was a problem hiding this comment.
이와 동시에 Page를 가리킨다는 것도 명시되어 있어야 close의 안전성이 완전히 설명될 것 같습니다.
There was a problem hiding this comment.
코드를 더 보다 보니 위 조건을 추가해도 여전히 충분하지 않은 것 같습니다. AllocatedPipe::close에서 페이지를 free하면 같은 주소를 가리키던 다른 AllocatedPipe의 invariant가 깨질 수 있습니다. 물론 Pipe::close의 결과가 true일 때만 free하며, Pipe::close의 결과가 true라는 조건이 같은 주소를 가리키는 다른 AllocatedPipe가 존재하지 않는다는 사실을 보장하므로 구현은 안전한 것이 맞습니다.
AllocatedPipe의 invariant에 "ptr이 가리키는 Pipe의 read_waitchannel과 write_waitchannel 중 하나가 false라면 자기 자신이 아닌 다른 AllocatedPipe가 존재하지 않는다"는(또는 이와 유사한 형태의) 조건이 추가로 있어야 할 것 같습니다.
저는 특별한 의견은 없습니다. 지금 상태도 크게 나쁘지는 않은 것 같은데, 바꿔서 더 간단해질 수 있다면 바꾸는 것도 좋다고 생각합니다. |
|
|
그렇네요. 간단한 부분을 제가 놓쳤네요. 말씀해주셔서 감사합니다! |
kernel-rs/src/pipe.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl AllocatedPipe { | ||
| pub fn alloc() -> Result<(RcFile<'static>, RcFile<'static>), ()> { | ||
| let page = kernel().alloc().ok_or(())?; | ||
| let ptr = page.into_usize() as *mut Pipe; | ||
| let mut ptr = NonNull::new(page.into_usize() as *mut Pipe).expect("AllocatedPipe alloc"); |
There was a problem hiding this comment.
앞으로 NonNull 을 많이 사용하게 된다면 Page::into_non_null_ptr 같은 걸 만드는 것도 좋겠습니다. (https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.as_non_null_ptr)
|
travis1829
changed the title
|
bors r+ |
362: [Closes #365] Fix TODOs in pipe.rs r=Medowhill a=travis1829 Closes #365 * #356 에서의 변경사항 중 `Pin` API와 관련없는 내용만 따로 빼서 새롭게 PR을 만들었습니다. (주로 `Pipe`에 대한 변경사항입니다.) * `Pipe`의 `Drop` trait을 추가했고 `Copy`, `Clone` trait을 없앴습니다. * 추가로, `Pipe::copy`가 `unsafe`일 필요가 없는 것 같아서 없앴습니다. (혹시 `unsafe`이여야하는 이유가 있다면 말씀해주세요.) * kernel.rs에 있던 주석을 일부 지웠습니다. (#356 (comment)) Co-authored-by: travis1829 <[email protected]>
|
Build failed: |
|
bors retry |
362: [Closes #365] Fix TODOs in pipe.rs r=Medowhill a=travis1829 Closes #365 * #356 에서의 변경사항 중 `Pin` API와 관련없는 내용만 따로 빼서 새롭게 PR을 만들었습니다. (주로 `Pipe`에 대한 변경사항입니다.) * `Pipe`의 `Drop` trait을 추가했고 `Copy`, `Clone` trait을 없앴습니다. * 추가로, `Pipe::copy`가 `unsafe`일 필요가 없는 것 같아서 없앴습니다. (혹시 `unsafe`이여야하는 이유가 있다면 말씀해주세요.) * kernel.rs에 있던 주석을 일부 지웠습니다. (#356 (comment)) Co-authored-by: travis1829 <[email protected]>
|
Build failed: |
|
bors retry |
|
Build succeeded: |
ghost
merged commit 
Closes #365
PinAPI와 관련없는 내용만 따로 빼서 새롭게 PR을 만들었습니다. (주로Pipe에 대한 변경사항입니다.)Pipe의Droptrait을 추가했고Copy,Clonetrait을 없앴습니다.Pipe::copy가unsafe일 필요가 없는 것 같아서 없앴습니다. (혹시unsafe이여야하는 이유가 있다면 말씀해주세요.)