Bug 38408 - dbus_g_proxy_manager_unregister wrongly asserts if GetNameOwner failed
Summary: dbus_g_proxy_manager_unregister wrongly asserts if GetNameOwner failed
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: GLib (show other bugs)
Version: unspecified
Hardware: Other All
: medium major
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL: http://cgit.freedesktop.org/~smcv/dbu...
Whiteboard: NB#116862 review+
Keywords: patch
: 41339 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-06-17 06:58 UTC by Simon McVittie
Modified: 2011-09-29 10:11 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
dbus_g_proxy_manager_unregister: if GetNameOwner failed, don't assert (1.27 KB, patch)
2011-06-17 06:59 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2011-06-17 06:58:39 UTC
If GetNameOwner fails with an error that is not NameHasNoOwner, dbus_g_proxy_manager_unregister wrongly asserts that the proxy is in a list to which it was never added.

(Based on a patch from Janne Karhunen, in Maemo's dbus-glib; his patch warned in this situation, but if GetNameOwner failed with some other error, then we already warned about that anyway.)
Comment 1 Simon McVittie 2011-06-17 06:59:11 UTC
Created attachment 48101 [details] [review]
dbus_g_proxy_manager_unregister: if GetNameOwner failed, don't assert

got_name_owner_cb never adds the proxy to the unassociated_proxies list
if there is a D-Bus error other than NameHasNoOwner.
    
Based on a patch from Janne Karhunen, for Maemo.
    
Bug-NB: NB#116862
Comment 2 Cosimo Alfarano 2011-08-12 07:21:30 UTC
Without any reviewer hat: pseudo-review++

No reason to assert, also there are several other occurences of g_assert which would deserve a review.
Comment 3 Simon McVittie 2011-08-15 03:40:31 UTC
(In reply to comment #2)
> Without any reviewer hat: pseudo-review++

Thanks, fixed in git for 0.96

> No reason to assert, also there are several other occurences of g_assert which
> would deserve a review.

Care to name some? (Or review them?)

My branches for Bug #35766 and Bug #35767 might well fix some wrong assertions too.
Comment 4 Cosimo Alfarano 2011-08-16 03:13:37 UTC
(In reply to comment #3)
> Care to name some? (Or review them?)

I'm giving a look at them and I'll open another bug if I'll found out anything suspicious, they are probably unrelated to this one.

> My branches for Bug #35766 and Bug #35767 might well fix some wrong assertions
> too.

Cool, I'll check against those to avoid duplicate work, thanks
Comment 5 Cosimo Alfarano 2011-08-16 03:49:29 UTC
(In reply to comment #3)
> Care to name some? (Or review them?)

False alarm, I thought I saw an assertion used in place of a condition, instead of a consistency check. Didn't find again, I probably misread the block.
Comment 6 Dan Williams 2011-09-29 10:11:31 UTC
*** Bug 41339 has been marked as a duplicate of this bug. ***


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.