| Summary: | Error handling improvements | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Kristian Rietveld <kris> |
| Component: | GLib | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | John (J5) Palmieri <johnp> |
| Severity: | normal | ||
| Priority: | medium | CC: | christian, gicmo, rob.taylor, smcv |
| Version: | unspecified | Keywords: | patch |
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Watch server_set_data failure in dbus__server_setup_with_g_main
Assert that marshalling in dbus_g_method_return succeeded Verify call in dbus glib test Show warning if marshalling in dbus_g_method_return fails Verify Increment call in dbus-glib-test Get member name from the method call message, not the reply, for warnings |
||
|
Description
Kristian Rietveld
2010-08-30 04:00:22 UTC
Created attachment 38288 [details] [review] Watch server_set_data failure in dbus__server_setup_with_g_main Created attachment 38289 [details] [review] Assert that marshalling in dbus_g_method_return succeeded Created attachment 38290 [details] [review] Verify call in dbus glib test Review of attachment 38289 [details] [review]: ::: dbus/dbus-gobject.c @@ +2713,3 @@ } + marshalled = _dbus_gvalue_marshal (&iter, &value); + g_assert (marshalled); Maybe the call to _dbus_gvalue_marshal() should be in an else block; and we should have a g_critical() with some context rather than an assertion? c.f. emit_signal_for_registration() Review of attachment 38289 [details] [review]: ::: dbus/dbus-gobject.c @@ +2713,3 @@ } + marshalled = _dbus_gvalue_marshal (&iter, &value); + g_assert (marshalled); Maybe the call to _dbus_gvalue_marshal() should be in an else block; and we should have a g_critical() with some context rather than an assertion? c.f. emit_signal_for_registration() The other two patches look good. Created attachment 41068 [details] [review] Show warning if marshalling in dbus_g_method_return fails Updated patch for dbus_g_method_return, so it warns instead of asserting. Created attachment 41069 [details] [review] Verify Increment call in dbus-glib-test This patch actually uses 'lose' as used elsewhere in the test, instead of assert. Review of attachment 41069 [details] [review]: Looks fine, r+ from me. Review of attachment 38288 [details] [review]: Looks fine. r+ from wjt already, r+ from me too. Review of attachment 41068 [details] [review]: ::: dbus/dbus-gobject.c @@ +2950,3 @@ + if (!_dbus_gvalue_marshal (&iter, &value)) + g_warning ("failed to marshal parameter %d for method %s", + i, dbus_message_get_member (reply)); Looks good in principle, but replies don't have a member name. I think you want dbus_message_get_member (context->message), or just context->method. Created attachment 41622 [details] [review] Get member name from the method call message, not the reply, for warnings Attachment 41069 [details] and Attachment 38288 [details] pushed to master. I attach a patch to fix the warning for Attachment 41068 [details], to be applied after that one. Fixed in git for 0.96, based on IRC review from Cosimo Alfarano and no veto from reviewers. |
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.