Bug 101481

Comment on attachment 132021 [details] [review]
Fix dbus_message_unref() is not called

Review of attachment 132021 [details] [review]:
-----------------------------------------------------------------

This looks good, thanks. One minor comment.

::: test/name-test/test-pending-call-disconnected.c
@@ +1,4 @@
> +/**
> +* Test to make sure that pending calls unref error messages
> +* when blocked after disconnected.
> +**/

It would be good to add a license header here (the fact that some of the other unit tests omit them is a bug).

@smcv, which is the preferred license header formatting to use at the moment?

(In reply to Philip Withnall from comment #1)
> @smcv, which is the preferred license header formatting to use at the moment?

(I have not reviewed the patch in any detail)

For code derived only from files with a MIT/X11 license grant (like test/dbus-daemon.c) or for new test code from scratch, the MIT/X11 license grant as used in test/dbus-daemon.c is preferred.

For code that is a derivative work of the rest of dbus please use the same GPL-2+|AFL-2.0 license grant as dbus/dbus-connection.c.

I'd prefer new tests to look more like the ones that use GLib (test/dbus-daemon-eavesdrop.c is a relatively simple example) and not be in test/name-test/, but tests in an older style in name-test/ are better than no tests.

Comment on attachment 132021 [details] [review]
Fix dbus_message_unref() is not called

Review of attachment 132021 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-connection.c
@@ +2476,4 @@
>  
>        /* on OOM error_msg is set to NULL */
>        complete_pending_call_and_unlock (connection, pending, error_msg);
> +      dbus_message_unref (error_msg);

This needs to be guarded by

if (error_msg != NULL)

because in the coding conventions used by dbus, it is wrong to unref NULL. The comment 2 lines earlier documents that error_msg might be NULL, and if you look at the implementation of generate_local_error_message() you'll see that it's true.

::: test/name-test/test-pending-call-disconnected.c
@@ +82,5 @@
> +
> +  printf (count == 0 ? "ok\n" : "not ok # Not all refs were unrefed ***\n");
> +
> +  printf ("# Testing completed\n1..1\n");
> +  exit (0);

Please exit (1) if unsuccessful (which in this case means count != 0).

Created attachment 132071 [details] [review]
Fix unref NULL

Created attachment 132072 [details] [review]
Fix exit (0) even if unsuccessful

Philip and Simon, thank you for the comments.

(In reply to Simon McVittie from comment #2)
> For code derived only from files with a MIT/X11 license grant (like
> test/dbus-daemon.c) or for new test code from scratch, the MIT/X11 license
> grant as used in test/dbus-daemon.c is preferred.

It's derived from test-pendinc-call-dispatch.c which is not clear about license grant.
Maybe MIT/X11 is the right choice?

> I'd prefer new tests to look more like the ones that use GLib
> (test/dbus-daemon-eavesdrop.c is a relatively simple example) and not be in
> test/name-test/, but tests in an older style in name-test/ are better than
> no tests.

Oh I'm sorry but I was not aware of that.
I decided where to write it reading the line 287 of HACKING.

(In reply to Simon McVittie from comment #3)
> This needs to be guarded by
> if (error_msg != NULL)
> 
> Please exit (1) if unsuccessful (which in this case means count != 0).

Thank you for the comments.
I attached additional patches.

(In reply to shin1morita from comment #6)
> It's derived from test-pendinc-call-dispatch.c which is not clear about
> license grant.

Judging by its git history, it was written in 2006 by John Palmieri (Red Hat), and since then it has only received trivial patches (which are probably not copyrightable) from Kjartan Maraas, Marcus Brinkmann, David Zeuthen (Red Hat), Simon McVittie (Collabora), Philip Withnall (Endless).

Red Hat have agreed in the past that their dbus contributions could be relicensed under the MIT/X11 license, so if you are OK with doing the same, I think we can use the same MIT/X11 license grant as test/dbus-daemon.c, with:

Copyright © 2006 Red Hat
Copyright © 2017 (your name)

I'll add a license grant to test-pending-call-dispatch.c while we're looking at that file anyway.

Created attachment 132080 [details] [review]
test-pending-call-dispatch: Add copyright and license grant

According to git history, this test was written in 2006 by Red Hat
employee John Palmieri and has received only trivial changes since
then. Red Hat gave permission in 2007 for their contributions to
be relicensed under the MIT/X11 license. We cannot take advantage
of that permission to relicense the core library or the dbus-daemon
from GPL-2+|AFL-2.0 to MIT/X11, because one early copyright holder
(CodeFactory AB) could not be traced, but we might as well use a
permissive license for simple test code that has not had
CodeFactory AB contributions.

(In reply to shin1morita from comment #7)
> (In reply to Simon McVittie from comment #3)
> > This needs to be guarded by
> > if (error_msg != NULL)
> > 
> > Please exit (1) if unsuccessful (which in this case means count != 0).
> 
> Thank you for the comments.
> I attached additional patches.

Please squash the additional patches into the original one, upload a new version of that patch, and mark all the old ones as obsolete in Bugzilla.

Comment on attachment 132080 [details] [review]
test-pending-call-dispatch: Add copyright and license grant

Review of attachment 132080 [details] [review]:
-----------------------------------------------------------------

++

::: test/name-test/test-pending-call-dispatch.c
@@ +1,2 @@
> +/*
> + * Copyright © 2006 Red Hat Inc.

I assume this UTF-8 corruption is Bugzilla, rather than the patch itself.

Created attachment 132085 [details] [review]
Squashed: Fix dbus_message_unref() is not called

Squashed first three patches, copyright and license grant as well.

(In reply to Simon McVittie from comment #8)
> Red Hat have agreed in the past that their dbus contributions could be
> relicensed under the MIT/X11 license, so if you are OK with doing the same,
> I think we can use the same MIT/X11 license grant as test/dbus-daemon.c,
> with:
> 
> Copyright © 2006 Red Hat
> Copyright © 2017 (your name)

Of course, I'm OK with it.
I attached the new patch which contains the MIT/X11 license grant and copyright in test-pending-call-disconnected.c.

Thank you.

(In reply to Philip Withnall from comment #10)
> Please squash the additional patches into the original one, upload a new
> version of that patch, and mark all the old ones as obsolete in Bugzilla.

I squashed them and attached the new patch.
Please check it out.

Thank you.

Comment on attachment 132085 [details] [review]
Squashed: Fix dbus_message_unref() is not called

Review of attachment 132085 [details] [review]:
-----------------------------------------------------------------

This looks good to commit with or without the change below, depending on whether smcv applies the patch before you update it (or if he finds other review issues).

::: test/name-test/test-pending-call-disconnected.c
@@ +111,5 @@
> +    }
> +  printf ("ok\n");
> +
> +  printf ("# Testing completed\n1..1\n");
> +  exit (0);

Nitpick: the code would be slightly cleaner if you combined the final two printf() calls (in the success case) and moved them into an `else` block:

if (count != 0)
  {
    …
  }
else
  {
    printf ("ok\n#Testing completed\n1..1\n");
    exit (0);
  }
}

(In reply to Philip Withnall from comment #15)
> This looks good to commit with or without the change below, depending on
> whether smcv applies the patch before you update it (or if he finds other
> review issues).
> 
> Nitpick: the code would be slightly cleaner if you combined the final two
> printf() calls (in the success case) and moved them into an `else` block:
> 
> else
>   {
>     printf ("ok\n#Testing completed\n1..1\n");
>     exit (0);
>   }

Oh, thank you. I agree with you.
I would replace the patch with new one.

Created attachment 132115 [details] [review]
Revised: Fix dbus_message_unref() is not called

Merged Philip's suggestion.

Thanks, fixed in git for 1.10.20 and 1.11.14.

Thank you.