Bug 39230 - [activation] update_desktop_file_entry always indicates error
Summary: [activation] update_desktop_file_entry always indicates error
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.4.x
Hardware: Other All
: low minor
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks: 39231
  Show dependency tree / graph
 
Reported: 2011-07-14 10:59 UTC by Simon McVittie
Modified: 2011-09-21 03:41 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:

Attachments
[PATCH 1/3] update_desktop_file_entry: don't leak file_path on one particular OOM (978 bytes, patch)
2011-07-21 05:23 UTC, Simon McVittie 		Details | Splinter Review
PATCH 2/3] update_desktop_file_entry: if the service name already existed, set error (1.37 KB, patch)
2011-07-21 05:23 UTC, Simon McVittie 		Details | Splinter Review
[PATCH 3/3] update_desktop_file_entry: initialize return value properly, and actually return it (1.07 KB, patch)
2011-07-21 05:23 UTC, Simon McVittie 		Details | Splinter Review
View All

Description Simon McVittie 2011-07-14 10:59:38 UTC 
gcc -Werror=unused reports that update_desktop_file_entry::retval is never used.

This means that its callers always go into their error-handling code paths!

This may have consequences for correct service activation.
Comment 1 Simon McVittie 2011-07-21 05:22:30 UTC 
My fault, this regressed when I fixed Bug #33126 (consistently freeing things on both success and failure in that function).

However, neither of that function's callers treat errors differently, unless the error is OOM, so this has no practical effect.

I did spot a related memory leak while fixing it, though...
Comment 2 Simon McVittie 2011-07-21 05:23:08 UTC 
Created attachment 49378 [details] [review]
[PATCH 1/3] update_desktop_file_entry: don't leak file_path on one  particular OOM

Revenge of #33126: most, but not all, temporary variables were freed on
this code path.
Comment 3 Simon McVittie 2011-07-21 05:23:32 UTC 
Created attachment 49379 [details] [review]
PATCH 2/3] update_desktop_file_entry: if the service name already  existed, set error

If we're going to return FALSE for this (which has apparently always
been the case), then we should set an error properly.
Comment 4 Simon McVittie 2011-07-21 05:23:59 UTC 
Created attachment 49380 [details] [review]
[PATCH 3/3] update_desktop_file_entry: initialize return value  properly, and actually return it

Since 1.4.4 (commit 75cfd97f) this function always returned FALSE. As far
as I can see this was actually harmless, because both of its callers
ignore any error that is not NoMemory (and treat it the same as success).
Comment 5 Will Thompson 2011-09-21 01:36:24 UTC 
Review of attachment 49378 [details] [review]:

Looks fine.
Comment 6 Will Thompson 2011-09-21 01:42:10 UTC 
Review of attachment 49379 [details] [review]:

++
Comment 7 Will Thompson 2011-09-21 01:43:54 UTC 
Review of attachment 49380 [details] [review]:

Ha. Looks good.
Comment 8 Simon McVittie 2011-09-21 03:41:04 UTC 
Fixed in git for 1.4.16 and 1.5.8, thanks

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.