Introduce close_safely helper function #763
Conversation
|
please signoff your commit |
mnissler-rivos
force-pushed
the
close_safely
branch
from
2c9fdfb
to
7374953
Compare
mnissler-rivos
force-pushed
the
close_safely
branch
from
7374953
to
b9f9ef5
Compare
The helper function centralizes some extra checks and diligence desired by many/most current code paths but currently inconsistently applied. This includes bypassing the close call when the file descriptor is -1 already, resetting the file descriptor variable to -1 after closing, and preserving errno. All calls to close are replaced by close_safely. Some warning log output is lost over this, but it doesn't seem like this was very useful anyways given that Linux always closes the file descriptor anyways. Signed-off-by: Mattias Nissler <mnissler@rivosinc.com>
mnissler-rivos
force-pushed
the
close_safely
branch
from
b9f9ef5
to
86f23ad
Compare
I can still see the old commits, and the "Compare" links on the right show what has changed in a push. "View reviewed changes" seems broken though - is that what you are refering to? Anyways, happy to follow whatever process you use for this project. Requested changes go into additional commits then? |
|
"show changes since last review" gets broken. In addition, any context for previous review comments is lost. Obviously for a small change it's no big deal, but the next one will be different! So yeah, during review, we use additional commits, and then "squash and merge" at the very end to have a single commit per logical change without a bunch of noise. |
The helper function centralizes some extra checks and diligence desired by many/most current code paths but currently inconsistently applied. This includes bypassing the close call when the file descriptor is -1 already, resetting the file descriptor variable to -1 after closing, and preserving errno.
All calls to close are replaced by close_safely. Some warning log output is lost over this, but it doesn't seem like this was very useful anyways given that Linux always closes the file descriptor anyways.