odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.12k stars 550 forks source link

fix(os2): check for 0 bytes read and return EOF #3776

Closed IllusionMan1212 closed 2 weeks ago

IllusionMan1212 commented 2 weeks ago

Current os2 file_stream implementation for read on linux never returns an EOF error when reaching the end of file.

Linux's read syscall returns 0 read bytes if the end of file is reached: https://man7.org/linux/man-pages/man2/read.2.html

This also matches the implementation of the os package as evident by core/os/stream.odin:30

gingerBill commented 2 weeks ago

This doesn't handle the case where len(p) == 0.

The question is, how should that be handled?

IllusionMan1212 commented 2 weeks ago

This doesn't handle the case where len(p) == 0.

The question is, how should that be handled?

It seems like the os package doesn't handle len(p) == 0 either.

Maybe len(p) == 0 should return io.Error.Short_Buffer. What do you think?

gingerBill commented 2 weeks ago

Ignore package os, it's awful.

But is that a short buffer or just an empty one? It's not necessarily an error.

laytan commented 2 weeks ago

IMO that should just return 0, nil

Kelimion commented 2 weeks ago

Reading into an empty buffer should always succeed, imo.

IllusionMan1212 commented 2 weeks ago

Personally I think an .Empty_Buffer error would be better since you're not getting anything useful out of reading into an empty buffer. But if everyone thinks an empty read should succeed then I'm ok with that too.

I could update the condition to be if len(p) != 0 && n == 0 && ferr == nil to distinguish EOF and empty reads.

gingerBill commented 2 weeks ago

It should be return 0, nil on an empty buffer.

IllusionMan1212 commented 2 weeks ago

Changed the check for EOF to be in the _read and _read_at functions. This should be good to go now.