[PATCH] do not call _dbus_warn_check_failed on checks

[Daniel Stone]

On Tue, Nov 14, 2006 at 12:43:46PM -0500, Havoc Pennington wrote:
> Daniel Stone wrote:
> >>If you want to change the API, argue to change the API. Nothing to do 
> >>with what happens on failed checks.
> >
> >Okay:
> >Change the API.
> 
> Good, this is the right discussion.
> 
> Here is why the API won't change to return a "you passed in NULL" error 
> code from every function:
> 
>  - it will double the size of the API since we have to keep the
>    existing one and then add variants like
>    dbus_connection_send_with_error() in addition to
>    dbus_connection_send()

libdbus-2.

>  - nobody will check for these errors (they don't on Windows and other
>    APIs that do this)

So what?  Why does that prevent you from offering this possibility?
(People will.  Not everyone, obviously.  But they will.)

>  - it means we *can't* warn about the NULL, which means we'll just
>    silently accept the NULL even though it's very likely a bug.
>    We can't print warnings about an error that's returned to
>    the caller because if we return an error the caller is responsible
>    for it.

If by 'accept', you mean 'reject', then you're right.

>  - it makes the already low-level API even harder to use by adding
>    more args to every function (or eliminating other uses for
>    the return value, e.g. we have to have
>    HRESULT dbus_message_new(DBusMessage **message_p) instead of
>    DBusMessage* dbus_message_new(void) )

Or DBusMessage *dbus_message_new(DBusError *), where the error param may
be null if you simply don't care, and a return of NULL indicates an
error.  If you care about whether this error is OOM or other, than you
pass in an error param.  Simple.

>  - it makes it unclear which functions can really fail for reasons
>    that should be checked, and which only fail due to a programming
>    error - when using the Windows API, you always have to read
>    the docs on this to decide if there are any HRESULT that can
>    sensibly be handled at all (not that MS documents it properly).
>    With dbus, if there is a DBusError you can be confident that
>    you should be checking it and not ignoring it.

Given you need to read the docs to know to use dbus_connection_unref,
instead of the rather intuitively named dbus_connection_close, I'd say
reading the docs to find out where Havoc does and doesn't consider the
error important, isn't such a bad thing.

>  - it's already possible to write a high-level binding that
>    verifies all the preconditions in advance (e.g. checks
>    for NULL) if someone really wants that; in fact for
>    just the NULL checks you can write this "binding" as
>    only a header file full of macros. This will avoid any
>    possibility that libdbus ever warns or exits.

Yes, hence libebus.  I think Alp also did a C version of his DBus#
stuff, which I'd definitely be interested in.  But it seems a shockingly
poor idea, particularly as the constraints may change over time.  Why
bother desyncing and implementing it in two places, rather than changing
_dbus_check or whatever to take a return value, a la g_return_if_fail?

>  - whatever you may think, it is a common and widely-accepted practice
>    to have functions without a way to report errors when those
>    functions have no error cases that can happen in a correct program.

These functions do not call abort.

>  - if we want to make "bug in my code" behavior configurable, it's
>    better to have a global way to set the "bug/log handler function"
>    than to screw up the whole API with error returns. Or just
>    make it configurable, say via an environment variable that changes
>    the behavior when a bug is detected. Kind of like the one we have.

Environment variables are a horrific API.  If you want to be truly
useful, give me dbus_never_abort_my_program().

I still don't see why you see error returns as 'screw[ing] up the whole
API'.  It would at least give us a consistent API, rather than the
current random hodgepodge where some functions take a GError, some
don't, some may fail randomly even without a GError, others always
succeed, ...

> If you want to write all your code like:
> 
>  result = foo();
>  if (result == BUG_IN_MY_CODE)
>     /* something */ ;
>  result = bar();
>  if (result == BUG_IN_MY_CODE)
>     /* something */;
>  result = baz();
>  if (result == BUG_IN_MY_CODE)
>     /* something */;
> 
> Have fun, but don't expect anyone else to want to do that!

Thanks for the strawman again, but that's not actually how I want to
write my code.

message = dbus_message_new();
if (!message)
    dont_continue_or_we_will_obviously_crash();
iter = dbus_iter_...();
if (!iter)
    ...();
foo();
bar();
baz();
if (!dbus_message_send(whatever))
    printf("message didn't send, y'all\n");

Apparently being careful is _so_ yesterday, which is why we have
libraries do it for us.  And call abort().  Call me old-fashioned if you
will, but that's not an approach I fail for applications, let alone
something that directly whacks the hardware, and will take your session
down if it dies.

I admire your commitment to correctness everywhere.  If you expended all
this energy towards running everything with fatal checks enabled
(including all GLib checks) and filed lots of bugs, most with patches,
then that would be fantastic.  But unfortunately, forcing this default
behaviour isn't very useful, except to decrease overall perception of
quality, and penalise people who are generally capable of coding but are
prone to making the odd typo (e.g. for (i = 0; i < foo; i++, pMap)),
because you're optimising for people who not only make typos, but do not
practise sound coding style, and refuse to code properly.

It's kind of a defeatist, and self-fulfilling, optimisation.  If you
optimise for crap, you're going to get crap.

> If you want to write a new libdbus or a libdbus binding that gives 
> people the ability to write code like that, please don't let me stop 
> you, but I wouldn't expect it to be wildly popular.

Given that no-one's apparently supposed to ever use libdbus anywhere (an
assertion I'm coming to understand with ever-growing clarity), wild
popularity isn't one of my great concerns.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.freedesktop.org/archives/dbus/attachments/20061114/7b7085e8/attachment.pgp