Bug 39230

Summary:	[activation] update_desktop_file_entry always indicates error
Product:	dbus	Reporter:	Simon McVittie <smcv>
Component:	core	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	John (J5) Palmieri <johnp>
Severity:	minor
Priority:	low	CC:	cosimo.alfarano, hp, will
Version:	1.4.x	Keywords:	patch
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Bug Depends on:
Bug Blocks:	39231
Attachments:	[PATCH 1/3] update_desktop_file_entry: don't leak file_path on one particular OOM PATCH 2/3] update_desktop_file_entry: if the service name already existed, set error [PATCH 3/3] update_desktop_file_entry: initialize return value properly, and actually return it

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.