Closing files after opening them.

[smoe]

This patch is with Debian for a really long time, likely to have its origins also in the cppcheck and/or clang runs. Just like PR #3284, this also addresses the reported leaking of file descriptors (#1175).

Anchor: #3260

[davidpanderson]

Closing files before exiting a program isn't needed.

The semantics of MFILE are that if you open() you have to close().
No need to do it in the destructor
(this is used only in cs_statefile.cpp)

[smoe]

Closing files before exiting a program isn't needed.

Well, the computer does not break, but BOINC is running for a long time and there has been that report on a file descriptor leakage for other installations. If the file does not need to be reopened a few seconds later - just close it and free the resources.

The semantics of MFILE are that if you open() you have to close().
No need to do it in the destructor
(this is used only in cs_statefile.cpp)

I disagree. A destructor encountering the left-open file should close it. If the semantics of that class suggest otherwise, it should still close it and send a request to stderr to get this fixed. If there is a forwarding of responsibilities to close that file by some copy operator of some sort, then this needs to be catered for.

[Germano0]

In my opinion we should also check the return value of fclose and close functions

[brevilo]

I agree with the PRs motivation and intentions but haven't yet verified the logic/code paths. What needs fixing either way is the indentation that currently a bit off.

[smoe]

There is more to the PR than the closing of files. @davidpanderson is however right that the file-closing concerns are indeed driven by short-lived applications. Feel free to cut that. However, from my POV I do not care if it is a long-running process or a short-running one. Did not even look - otherwise I may not even have submitted it to avoid discussions. I always close what is no longer needed. And free what is no longer needed. ASAP, "P" as in "there is no semantics associated with 'having a lock on a file'". That way the code is also allowed to move into other parts of BOINC. There is no unnecessary context required to understand that part of the code. And I admittely sense some satisfaction over that elegance.

[ChristianBeer]

If you fix the indentation this is ready to be merged. My POV is that closing a file even before ending a short-lived program shows that the developer intentionally thought about where to close the file. The same goes for MFILE. What if a developer forgot about the semantics and didn't close. The destructor would take care of that.

[codecov]

Codecov Report

Merging #3285 (f9d53b9) into master (93f4dde) will increase coverage by 13.36%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #3285       +/-   ##
=============================================
+ Coverage      9.27%   22.63%   +13.36%     
- Complexity        0      756      +756     
=============================================
  Files            36      101       +65     
  Lines          5992     7319     +1327     
  Branches          0     1516     +1516     
=============================================
+ Hits            556     1657     +1101     
- Misses         5436     5550      +114     
- Partials          0      112      +112     

Impacted Files	Coverage Δ	Complexity Δ
db/boinc_db_types.h
lib/coproc.cpp
lib/sched_msgs.cpp
lib/miofile.h
lib/sched_msgs.h
db/db_base.cpp
sched/sched_shmem.cpp
lib/parse.h
sched/sched_limit.h
lib/url.cpp
... and 126 more

[smoe]

The indenting should be fine now - typical tabs vs blanks. Isn't there possibly a way to have the auto-indented?
Anyway - let me try to rebase this branch prior to merging anything because of the small commits - seems like I should have done that locally.