|
Description
Simon McVittie
2011-01-14 10:17:36 UTC
Created attachment 42049 [details] [review] free more stuff in dbus-cleanup-sockets Doesn't look very important, since it's in an ancillary tool. Not reviewed. Created attachment 42050 [details] [review] handle OOM in nonce authentication Looks right at first glance, but not reviewed in detail yet. Created attachment 42051 [details] [review] handle failure to register inotify shutdown function Looks OK at first glance, although more cleanup/unwinding might be required. Created attachment 42053 [details] [review] Free things in update_desktop_file_entry() I think this can sometimes double-free exec? Similar to one of my patches on Bug #33126. Created attachment 42054 [details] [review] free matchmaker itself if bus_matchmaker_new fails Looks OK at first glance. Created attachment 42055 [details] [review] clean up nonce file more reliably in _dbus_server_new_for_tcp_socket I can't immediately see anything wrong but 4 unwinding labels is a bit much. Perhaps this could be refactored. Created attachment 42056 [details] [review] remove obviously-redundant code from list_concat_new Looks good to me. Created attachment 42057 [details] [review] Clean up credentials on OOM in process_config_first_time_only I would hope that we could move the _dbus_credentials_unref call to below the oom label rather than duplicating it. Created attachment 42058 [details] [review] test_server_setup: clean up server data Created attachment 42059 [details] [review] replace unreachable code with an assertion I haven't verified that this is actually unreachable... Created attachment 42060 [details] [review] dbus-send: turn unreachable code into an assertion Again, I haven't verified that this is unreachable. Created attachment 42061 [details] [review] [review-] writer_recurse_array: add an assertion Not sure what this is for; it's probably to shut Coverity up, more than anything else. Review of attachment 42050 [details] [review]: Committed. Review of attachment 42054 [details] [review]: Committed. Review of attachment 42051 [details] [review]: Committed. Review of attachment 42056 [details] [review]: Committed Review of attachment 42049 [details] [review]: This isn't really important - dbus-cleanup-sockets should terminate rapidly, at which point memory leaks become entirely academic - but the patch looks fine so we might as well have it. Committed. Review of attachment 42058 [details] [review]: ::: test/test-utils.c @@ -320,3 @@ nomem: - if (sd) - serverdata_free (sd); I'm guessing this was probably intended to fix a Coverity warning about unreachable code? It's the wrong fix: the bug here is that if we fail to set the watch/timeout functions, we return directly (without freeing @sd) instead of jumping to nomem to clean up. Rejected; I'll write a patch for the correct version. Comment on attachment 42053 [details] [review] Free things in update_desktop_file_entry() Obsoleted by patches 2/7 to 6/7 on Bug #33126. Created attachment 42543 [details] [review] dbus_connection_dispatch: remove dead code There's no way pending can be non-NULL here; if it was, we'd have jumped straight past this block (getting filters from the connection), because replies to pending calls don't go through filters. (This patch isn't particularly important, but it makes our library fractionally smaller, removes dead code that might confuse someone, and makes code analyzers shut up.) Created attachment 42544 [details] [review] dbus-send: remove minor dead code This version is more self-explaining and has fewer lines. :-) Review of attachment 42061 [details] [review]: It's not at all clear to me that this is correct. Either it's papering over a real bug, or there are a lot of redundant checks; analysis needed. Review of attachment 42055 [details] [review]: This one does look correct, although it could probably be clearer. Applied. The remaining non-obsoleted attachments are good to go, I think. I rewrote them, so review from someone else is required. well, my less-than-1p, since the patches are really simple :) but they look fine to me, without any developer hat on my head. Fixed in git for 1.4.10 (R-b: Cosimo), will be merged to master for 1.5.2 |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.