Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#10367 closed bug (no change required)

Fix Coverity CID 1032247 and 1032248: unchecked return value

Reported by: noryb009 Owned by: axeld
Priority: normal Milestone: R1/beta1
Component: Kits/Storage Kit Version: R1/Development
Keywords: gci2013 coverity Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Attachments (2)

Change History (11)

comment:1 Changed 6 years ago by noryb009

Has a Patch: set

comment:2 Changed 6 years ago by noryb009

Component: - GeneralKits/Storage Kit
Owner: changed from nobody to axeld

comment:3 Changed 6 years ago by umccullough

Can you update the patch with a small style fix - a space between "if" and "("

comment:4 Changed 5 years ago by waddlesplash

Keywords: coverity added
Milestone: R1R1/alpha5

comment:5 Changed 5 years ago by pulkomandy

This does not look like a good solution: the number of bytes actually written or read would then be lost, and it's more important to know that, rather than the fact that the seek back has failed.

comment:6 Changed 5 years ago by noryb009

Personally, I think an error code is more important than whatever is read - it would be a silent error, and can cause a confusing crash ("This read fails. Wait, the position is at the end of the file. How did it get there? Looks like ReadAt isn't emitting an error." - a possible future programmer, spending a long night fixing a bug).

From the be book (BPositionIO page):

It should return the number of bytes actually read, or an error code if something goes wrong.

So if something goes wrong (the seek back), the error number should be returned. However, the be book (and the API documentation) doesn't state that this functionality exists (that the position will be returned to the original location).

If the seek back isn't guaranteed, then it shouldn't happen at all (faster, one less seek, programmers can do it themselves if needed). Alternatively, it should be guaranteed and defined (doesn't break the current API).

Until we are sure we want to either define it or remove it, we should return an error if it fails to avoid the scenario above.

comment:7 Changed 5 years ago by bonefish

In practice seeking back cannot fail. The arguments (fFile, oldPosition) are known to be valid. If someone else concurrently truncates or removes the file, fseeko() will still succeed. The only possible error situation is, when the file is closed concurrently. However, in this case it is irrelevant that the seek operation failed, since it isn't possible to do any further reads/writes anyway.

Since that would involve a programming error on the API user's side, calling debugger() or logging the error would be an option. I'd still return success, though.

comment:8 Changed 5 years ago by pulkomandy

Resolution: no change required
Status: newclosed

comment:9 Changed 5 years ago by pulkomandy

Milestone: R1/alpha5R1/beta1
Note: See TracTickets for help on using tickets.