usize n in Pipe::read() should be u32

[anemoneflower]

rv6/kernel-rs/src/pipe.rs

Lines 43 to 44 in 5bbd4f0

	//TODO : `n` should be u32
	pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {

[travis1829]

이미 해결된 TODO인것 같습니다.

• 일단, 이 TODO가 추가된 이유는 [closes #205] struct Pipe 리팩토링 #206 (comment) [closes #205] struct Pipe 리팩토링 #206 (comment) [closes #205] struct Pipe 리팩토링 #206 (comment) 입니다.

#210 에서 수정하다가 이 PR에서 남긴 TODO에 관하여 질문드립니다. @efenniht

n 은 읽어야 할 바이트의 수이므로 올바른 타입은 usize 로 보이는데, system call spec 이 32비트로 길이를 받는 것 같으므로, >u32 가 맞아 보입니다. 당장은 바꾸기 힘들고, File 을 고치면서 수정하도록 합시다. TODO 남겨주세요.

#206 (comment) 에서 Pipe::read의 n은 i32가 아닌 u32여야 한다고 하셨는데, 음수바이트만큼 읽을 수 없으니 u32여야 한다는 말씀에 동의합니다. (usize가 아닌 u32인 이유도 이해했습니다)

그런데 만약 n이 음수로 주어진다면 원래 의도는 pipe.c의 117번째줄처럼 아무것도 읽지 않는 것인데,
n을 as u32 타입으로 바꾼다면 의도치 않게 많은 바이트를 읽게 되지 않을까요? 이것을 생각하면 i32를 유지하는 것이 맞는 것 같아 질문드립니다.

rv6/kernel/pipe.c

Lines 102 to 127 in f84b36c

	int
	piperead(struct pipe *pi, uint64 addr, int n)
	{
	int i;
	struct proc *pr = myproc();
	char ch;
	acquire(&pi->lock);
	while(pi->nread == pi->nwrite && pi->writeopen){ //DOC: pipe-empty
	if(myproc()->killed){
	release(&pi->lock);
	return -1;
	}
	sleep(&pi->nread, &pi->lock); //DOC: piperead-sleep
	}
	for(i = 0; i < n; i++){ //DOC: piperead-copy
	if(pi->nread == pi->nwrite)
	break;
	ch = pi->data[pi->nread++ % PIPESIZE];
	if(copyout(pr->pagetable, addr + i, &ch, 1) == -1)
	break;
	}
	wakeup(&pi->nwrite); //DOC: piperead-wakeup
	release(&pi->lock);
	return i;
	}

Originally posted by @kimjungwow in #206 (comment)
다만, 이 TODO가 나올 당시에는 Pipe::read()의 n이 xv6와 비슷하게 다음과 같이 i32형이었습니다.

rv6/kernel-rs/src/pipe.rs

Lines 42 to 43 in 9407397

	//TODO : `n` should be u32
	pub unsafe fn read(&self, addr: usize, n: i32) -> i32 {

• 그래서, @efenniht 님께서 조언해주신대로 김정우님께서 다음과 같이 바꿨었습니다.
  848a811#diff-c5d2f315fd8272dfa0b9871ea6934cb67ab89cc9baf45c8b336cd401dd0738e8L135-R136
  848a811#diff-a988410e052d2279f32f7ea5358e9d65a85bd38dbcedf2be50c507c030a24268L43-R43
  이로써 TODO가 이미 해결된 것으로 보입니다.
• 다만, n이 현재는 u32가 아니라 usize로 바뀐 이유는 동일한 PR에서 김정우님이 u32를 usize로 바꿨기 때문입니다.
  6c1f1b3#diff-c5d2f315fd8272dfa0b9871ea6934cb67ab89cc9baf45c8b336cd401dd0738e8L136-R136
  6c1f1b3#diff-a988410e052d2279f32f7ea5358e9d65a85bd38dbcedf2be50c507c030a24268L43-R43
  • 개인적인 생각으로는, Pipe::read()와 Pipe::write() 모두 현재까지 성공적으로 읽은/쓴 byte 수를 Ok(n: usize)로 리턴하기때문에 차라리 n도 usize인게 낫다고 생각하셔서 이렇게 바꾸신 것 같습니다. (특히 n: u32면 Pipe:write()에서 쓸데없이 형변환하는 부분이 많이 생깁니다.)
• @efenniht 혹시 확인해주실 수 있으신가요? 괜찮으시면 TODO만 지우고 이 issue는 닫겠습니다.

[travis1829]

추가적으로, 이와는 별개로, File::read()/File::write()이 읽고싶은/쓰고싶은 byte의 수를 i32로 받는데 반해 성공적으로 읽은/쓴 byte 수는 u32가 아니라 usize로 리턴하는게 조금 걸리는 것 같습니다. 일반적으로는 당연히 usize가 u32보다 같거나 더 큰 범위를 커버하지만, 그렇지 않은 극단적인 경우도 있긴 하기 때문입니다.

[efenniht]

• 말씀하신 경우는 platform-dependent 하기 때문에 컴파일 시점에 검사할 수 있습니다: https://docs.rs/static_assertions/1.1.0/static_assertions/macro.assert_cfg.html#examples .
• 다만 지금의 xv6 코드는 riscv64 에서 돌아가는 것을 많은 곳에서 가정하고 있기 때문에 해당 assertion 을 추가하는 것이 큰 의미는 없어 보입니다.
• Platform-independent 한 core logic 을 분리한다면 해당 사항을 고려해볼만 하겠습니다.

[travis1829]

다만 지금의 xv6 코드는 riscv64 에서 돌아가는 것을 많은 곳에서 가정하고 있기 때문에 해당 assertion 을 추가하는 것이 큰 의미는 없어 보입니다.

제 생각에도 아직은 굳이 추가할 필요는 없을 것 같습니다. 다만, 교수님 말씀대로 혹시라도 OS를 embedded에 적용하려고 한다면 그때는 신경써야 할지도 모르겠습니다.

이와는 별개로, 이 TODO는 없애도 괜찮을 것 같나요? 이유는 위에 설명해놨습니다.

rv6/kernel-rs/src/pipe.rs

Lines 43 to 44 in 5bbd4f0

	//TODO : `n` should be u32
	pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {

[jeehoonkang]

archi-dependent한 부분에 주석 남겨주시면 감사드리겠습니다.
지금은 archi target이 하나이지만 나중에 분명히 확장할건데, 그때 주석이 있으면 큰 도움이 될 것 같습니다.
archi dependency 관련한 meta issue를 남기고, 그 issue를 TODO refer하면 될듯 합니다.

[efenniht]

이와는 별개로, 이 TODO는 없애도 괜찮을 것 같나요? 이유는 위에 설명해놨습니다.

rv6/kernel-rs/src/pipe.rs

Lines 43 to 44 in 5bbd4f0

	//TODO : `n` should be u32
	pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {

고민인데... usize 를 쓰는 것과 u32 를 쓰는 것이 다 설득력이 있습니다:

• usize: 연속된 메모리의 크기와 offset 을 다루는 것이므로 usize 가 자연스럽습니다. 그리고 Rust standard 의 Read::read 와 POSIX 표준 (fread) 모두 usize 와 size_t 를 주고받습니다.
• u32: xv6 는 하지만 int 를 주고받습니다. syscall 이 int 를 주고받는 것은 커널 내부의 function signature 보다는 더 강한 규약이니까 깨는게 맞는 지 잘 모르겠습니다.

지금은 어중간하게 되어 있네요:

• File::{read,write} 은 i32 를 인자로 받고, usize 를 리턴
• InodeGuard::{read,write} 은 u32 를 인자로 받고, usize 를 리턴

어느 한 쪽으로 정하는 게 좋겠습니다. cc @jeehoonkang

[jeehoonkang]

• kernel 내부에선 전부 usize로 하고,
• syscall과 연결하는 부분에서 u32 <-> usize 변환 해주는건 어떨까요? @efenniht

[efenniht]

#380 과 #381 을 만들었으므로 이 이슈는 닫도록 하겠습니다.