Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#10889 - Close leaked filehandles on plugin exit

Attached to Project: Rockbox
Opened by Frank Gevaerts (fg) - Saturday, 02 January 2010, 17:46 GMT+2
Last edited by Frank Gevaerts (fg) - Thursday, 11 February 2010, 20:21 GMT+2
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Rbutil SVN
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
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.
   close_plugin_leaked_filehandles.diff (2.6 KiB) 
 apps/plugin.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 4 deletions(-)

This task depends upon

Closed by  Frank Gevaerts (fg)
Thursday, 11 February 2010, 20:21 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  committed as r24598
Comment by Frank Gevaerts (fg) - Saturday, 02 January 2010, 20:15 GMT+2
The double close() check is reversed. I'll fix it later
Comment by Bertrik Sikken (bertrik) - Saturday, 02 January 2010, 21:14 GMT+2
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, 22:26 GMT+2
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, 22:31 GMT+2
This patch should fix the double close() check. Please review, bitwise logic seems to make me dizzy today...
   close_plugin_leaked_filehandles-2.diff (2.6 KiB) 
 apps/plugin.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 4 deletions(-)

Comment by amaury pouly (pamaury) - Friday, 08 January 2010, 23:08 GMT+2
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.
   close_plugin_leaked_filehandles.diff (4.1 KiB) 
 b/apps/plugin.c            |   81 ++++++++++++++++++++++++++++++++++++++++++++-
 b/firmware/export/config.h |    7 +++
 2 files changed, 86 insertions(+), 2 deletions(-)

Comment by Frank Gevaerts (fg) - Saturday, 09 January 2010, 14:28 GMT+2
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, 20:13 GMT+2
New version:
-add translation support
-remove LOGF_ENABLE
   close_plugin_leaked_filehandles-4.diff (4.7 KiB) 
 b/apps/lang/english.lang   |   28 +++++++++++++++
 b/apps/plugin.c            |   82 ++++++++++++++++++++++++++++++++++++++++++++-
 b/firmware/export/config.h |    7 +++
 3 files changed, 115 insertions(+), 2 deletions(-)

Comment by amaury pouly (pamaury) - Saturday, 09 January 2010, 20:47 GMT+2
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)
   close_plugin_leaked_filehandles-5.diff (3.9 KiB) 
 b/apps/plugin.c            |   82 ++++++++++++++++++++++++++++++++++++++++++++-
 b/firmware/export/config.h |    7 +++
 2 files changed, 87 insertions(+), 2 deletions(-)

Loading...