search

nutanix / libvfio-user

framework for emulating devices in userspace
BSD 3-Clause "New" or "Revised" License
168 stars 51 forks source link

Introduce close_safely helper function #763

Closed mnissler-rivos closed 1 year ago

mnissler-rivos commented 1 year ago

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.

jlevon commented 1 year ago

please signoff your commit

mnissler-rivos commented 1 year ago

Thanks - but please don't use force push during PR review, as it deletes all context from previous reviews, and I can't tell what changed and so on.

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?

jlevon commented 1 year ago

"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.