Bug 8365 - [patch] clarify function semantics
Summary: [patch] clarify function semantics
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: high normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
Keywords: patch
Depends on:
Reported: 2006-09-20 03:15 UTC by Kimmo Hämäläinen
Modified: 2006-11-16 03:17 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

proposed patch (11.90 KB, patch)
2006-09-20 03:15 UTC, Kimmo Hämäläinen
Details | Splinter Review

Description Kimmo Hämäläinen 2006-09-20 03:15:17 UTC
Here is a patch to clarify function semantics for dbus_connection_* public
functions. (If this is applied, I can check all of the public functions and send
more patches.)
Comment 1 Kimmo Hämäläinen 2006-09-20 03:15:52 UTC
Created attachment 7094 [details] [review]
proposed patch
Comment 2 Havoc Pennington 2006-09-20 04:08:47 UTC
This isn't right, because the "invalid parameters" return values are not in 
the API contract.

Say I have this function:

frobate(char *a, int b)
   return_val_if_fail(a != NULL, NULL);
   return_val_if_fail(b >= 1, NULL);

   return strdup_printf("%s: %d", a, 42 + b);

the docs are:
 - it returns the string "a: 42+b"
 - it returns NULL if there is not enough memory to allocate
   the returned string
 - a must not be NULL or the results are undefined
 - b must be >= 1 or the results are undefined

There are two _likely_ outcomes if you pass a=NULL or b<1, which are a zero 
return, or (with --disable-checks) the function just tries to use the invalid 
parameters, which means you could get a string like "(null): 42+b" on Linux, 
or a crash on Solaris where null for %s is not allowed.

Now say the function is:

frobate(char *a, int b)
   if (a == NULL)
      return NULL;
      return strdup_printf("%s: %d", a, 42 + b);

Now the docs are different, because a==NULL is a VALID parameter handled in a 
defined way. Now the docs are that if a is NULL, the function returns NULL. 
Note that in this case, OOM cannot be reliably detected.

return_if_fail() is just the same, conceptually, as assert(). It's a "friendly 
assert" that doesn't actually abort(), but it's still an assertion.
Comment 3 Kimmo Hämäläinen 2006-09-20 04:22:39 UTC
My point is that the programmer needs to know _exactly_ when and what is
returned. The API contract should be _told_ in the function comment (or where
are you telling it?). I can change the comment to 'results are undefined', but I
think it's even worse than saying what happens.

The cases when it crashes are not the problem (then it does not return).

So: the API contract must be told, or the programmer needs to guess -- and then
there is a risk of misunderstanding. E.g. the programmer could _count on_ the
fact that it crashes on invalid arguments (and test accordingly), but instead it
returns NULL.
Comment 4 Kimmo Hämäläinen 2006-09-20 04:52:27 UTC
Can you just think of these alternatives: A) explicitly telling in the function
comment that the results are undefined if invalid parameters are passed, or B)
not telling what happens if invalid parameters are passed.

I think alternative A would remind the programmer to validate the parameters in
case he really is relying on the result. In case of B, the programmer would
probably forget about the validity and/or trust in the _documented_ return value(s).

You can see how e.g. the man pages do pretty good work on the documentation.
They even say when the value of some pointer is undefined. The D-Bus API
documentation does nothing like that.
Comment 5 Havoc Pennington 2006-09-20 10:20:22 UTC
I think it would improve the docs to add "the interface name must be non-NULL 
and syntactically valid, or the results are undefined" or "the connection must 
not be NULL or the results are undefined"

It's actively misleading to document that there's any particular return value 
if the parameters are invalid, because for most parameters, it is not 
true. "undefined" is meant in a very literal sense here: the library may print 
a warning, crash, and/or return some junk value.
Comment 6 Kimmo Hämäläinen 2006-09-20 23:13:40 UTC
I agree almost all what you are writing, but I think we should avoid returning
'some junk value' at all costs. This is because it can only harm the user of the
library, it can never benefit the user. However, if we say that 'in the case
validation is compiled, NULL is returned for invalid parameter', then the user
could at least prepare for the case.

The alternative is that we never return on invalid parameters. I think changing
the semantics of the API when validation is or is not compiled is not good: it
would be better that return_val_if_fail() etc. would only print an error if
validation is compiled in (and later the function could crash -- but the user
would see the reason for the crash from the warning message). So, they would
never cause the function to return. That way the semantics of the function would
not change depending on compilation of checks and validations.
Comment 7 Havoc Pennington 2006-09-20 23:54:20 UTC
I would have no objection to changing to the equivalent of 
G_DEBUG=fatal_warnings for glib (i.e. it prints the warning, then aborts), at 
least by default. We might or might not add a dbus_set_fatal_warnings to let 
people go back to the less aggressive "only warn and return a possibly-safe 
but undefined value" mode.

I would say abort() instead of warn and continue, because continuing could 
have all kinds of unknown effects including buffer overflows or silently 
corrupting data.

--disable-checks is not encouraged for a typical dbus install; its purpose is 
for embedded systems worried about code size and other corner cases. Correct 
programs will not change behavior with --enable-checks vs. --disable-checks.
Comment 8 Kimmo Hämäläinen 2006-09-21 00:14:04 UTC
I agree. I also think that the Glib way of allowing non-fatal warnings (that
actually are dangerous bugs) is one big mistake that the Glib designers made. It
has caused us lot of trouble when developing the Nokia 770 (think about how to
force fixing the bugs when just making them fatal is _not_ an option due to the
internal processes).

I would strongly suggest that D-Bus only has fatal warnings (abort()) if they
are compiled in. The reason for this is that returning values in order to avoid
the crash is only helping to hide bugs in the client code -- so it's rather
dangerous than helpful.
Comment 9 John (J5) Palmieri 2006-10-11 08:18:23 UTC
So what is the status of this patch.  Do we add it or does it need changes? 
From the descussion it seems to need some changes.
Comment 10 Kimmo Hämäläinen 2006-10-12 00:32:49 UTC
I could make a new patch if Havoc agrees on the changes. As I said, I would
prefer that the semantics are the same, despite of what checks have been
compiled in (i.e. no new return values if checks are compiled). Failing check
could mean an optional warning followed by abort(). I would not return 'garbage
value' in case of a failing check.
Comment 11 Havoc Pennington 2006-10-12 04:22:08 UTC
abort or not doesn't affect the docs. We aren't going to support a defined 
behavior if people pass junk args. I think the abort() would be a good option 
to have (like glib's fatal warnings mode), but the docs are still "it's 
undefined" - it's not in the API contract.

If someone wants to be able to pass junk args, they can create a libdbus 
wrapper or binding that validates their args. But I don't want to maintain 
these guarantees in libdbus itself, the libdbus policy is that you must use 
the API correctly and as documented or the results are undefined.
Comment 12 Kimmo Hämäläinen 2006-10-13 02:35:17 UTC
I'm not asking for defined operation in case of invalid arguments (that's
impossible). My point is that the documented function returns should be the only
returns the user ever sees, because otherwise there is a risk of misinterpretation.
Comment 13 Kimmo Hämäläinen 2006-11-16 03:17:22 UTC
I think we could consider this fixed now that the library causes crash on
invalid arguments, because that keeps the semantics clear (no surprise returns).

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.