[1.x] StdIOCallback: return 0 on read/write errors#311
[1.x] StdIOCallback: return 0 on read/write errors#311robUx4 wants to merge 1 commit intoMatroska-Org:v1.xfrom
Conversation
That's what the IOCallback abstraction expects.
robUx4
changed the title
| assert(File!=nullptr); | ||
|
|
||
| size_t result = fread(Buffer, 1, Size, File); | ||
| if (feof(File) || ferror(File)) |
There was a problem hiding this comment.
Does this really make sense? If you're, say, at position 500 in a file of size 1000 & request to read 2KB, the file descriptor will have its EOF flag set, and you simply don't return the last 500 bytes that could & were actually read to the calling function, thereby losing data. You cannot expect a function to only request exactly as much as the file size indicates, and punishing it for it is not nice.
Or am I missing something here?
There was a problem hiding this comment.
You're right we should not punish for not knowing the size, but:
- this is how the read() method has been defined from the beginning (or since we moved to git)
- in most cases we do know the size we are reading, EBML length and ID are read byte by byte, most elements have a known size
- in Matroska only a few very large elements are allowed to have an unknown size (here we're talking libebml so maybe we shouldn't be this strict)
- the read() method doesn't have a way to report EOF or other kinds of errors, or we need to throw errors, but that's not documented nor handled by callers
There was a problem hiding this comment.
I get all that. For libEBML/libMatroska this sounds OK. I'm just worried that if any other program is using StdIOCallback this would be a grave behavior change for them. I actually don't know if there are any; MKVToolNix has its own I/O class derived from IOCallback.
There was a problem hiding this comment.
In VLC we have our own too, and I added the behaviour to match the documentation recently.
2 participants
That's what the IOCallback abstraction expects.