Rockbox

Tasklist

FS#10889 - Close leaked filehandles on plugin exit

Attached to Project: Rockbox
Opened by Frank Gevaerts (fg) - Saturday, 02 January 2010, 16:46 GMT
Last edited by Frank Gevaerts (fg) - Thursday, 11 February 2010, 19:21 GMT
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Rbutil SVN
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch makes plugin.c check for leaked file handles, and closes them if needed.
It can of course be trivially changed to panic instead.

On arm, there's a binsize cost of about 256 bytes.

Not actually tested yet.
This task depends upon

Closed by  Frank Gevaerts (fg)
Thursday, 11 February 2010, 19:21 GMT
Reason for closing:  Accepted
Additional comments about closing:  committed as r24598
Comment by Frank Gevaerts (fg) - Saturday, 02 January 2010, 19:15 GMT
The double close() check is reversed. I'll fix it later
Comment by Bertrik Sikken (bertrik) - Saturday, 02 January 2010, 20:14 GMT
I like the patch. I'd like any problem found with leaked file descriptors or double close to be more visible than just a logf, for example using a splash so users are triggered more to report a problem.
Comment by Frank Gevaerts (fg) - Saturday, 02 January 2010, 21:26 GMT
I agree that logf is too weak, but I'm not sure if a splash isn't too annoying.
Comment by Frank Gevaerts (fg) - Saturday, 02 January 2010, 21:31 GMT
This patch should fix the double close() check. Please review, bitwise logic seems to make me dizzy today...
Comment by amaury pouly (pamaury) - Friday, 08 January 2010, 22:08 GMT
New version, the check can be disabled by not defining HAVE_PLUGIN_CHECK_OPEN_CLOSE in config.h.
Also wipe out wrappers if not needed and add a yesno dialog to inform the user to report the problem.
Comment by Frank Gevaerts (fg) - Saturday, 09 January 2010, 13:28 GMT
Some comments:

- I don't think any source file should have LOGF_ENABLE by default.
- Unifying the logf and GUI message strings should save some binsize. Not sure if it's worth it though.
- Shouldn't the GUI messages be translated?
Comment by amaury pouly (pamaury) - Saturday, 09 January 2010, 19:13 GMT
New version:
-add translation support
-remove LOGF_ENABLE
Comment by amaury pouly (pamaury) - Saturday, 09 January 2010, 19:47 GMT
New version:
-reuse an existing translation instead of a new one and had a little text (it could be change to an error number for example)

Loading...