Bug 66068

Created attachment 81246 [details] [review]
[PATCH 1/5] dbus-launch: fix coding style

Created attachment 81247 [details] [review]
[PATCH 2/5] dbus-launch: align document

Created attachment 81248 [details] [review]
[PATCH 3/5] dbus-launch: check SIGHUP definition and free args on  failure

Created attachment 81249 [details] [review]
[PATCH 4/5] dbus-launch: do not verbose output if build with verbose  mode disabled

Created attachment 81250 [details] [review]
[PATCH 5/5] dbus-launch: fix syntax usage

Comment on attachment 81246 [details] [review]
[PATCH 1/5] dbus-launch: fix coding style

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

Looks good, apart from:

::: tools/dbus-launch.c
@@ +756,5 @@
>        if (envvar == NULL || args == NULL)
>          goto oom;
>  
> +      args[0] = xstrdup (runprog);
> +        if (!args[0])

This extra indent doesn't make sense?

Comment on attachment 81247 [details] [review]
[PATCH 2/5] dbus-launch: align document

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

Makes sense.

Comment on attachment 81248 [details] [review]
[PATCH 3/5] dbus-launch: check SIGHUP definition and free args on  failure

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

::: tools/dbus-launch.c
@@ +479,2 @@
>    sigaction (SIGHUP,  &act, NULL);
> +#endif

What platform "is Unix" but doesn't have SIGHUP?

(The Windows version of dbus-launch is completely separate.)

@@ +768,4 @@
>          {
>            size_t len = strlen (argv[remaining_args+i-1])+1;
>            args[i] = malloc (len);
> +          if (!args[i]) {

That's not our coding style (I realise dbus-launch is a mess, but we use GNU indentation fairly consistently).

@@ +769,5 @@
>            size_t len = strlen (argv[remaining_args+i-1])+1;
>            args[i] = malloc (len);
> +          if (!args[i]) {
> +            while (i > 1)
> +              free(args[--i]);

Maybe. I'd have to check the logic.

It doesn't really matter, since presumably we're about to exit anyway...

Comment on attachment 81249 [details] [review]
[PATCH 4/5] dbus-launch: do not verbose output if build with verbose  mode disabled

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

::: tools/dbus-launch.c
@@ +183,5 @@
> +void
> +verbose (const char *format,
> +         ...)
> +{
> +  ; /* do nothing */

No empty statements (redundant semicolons) please; they're not valid C, strictly speaking, and some compilers fail to compile them. This

   verbose (const char *format,
            ...
   {
   }

should be sufficient (with or without a comment, as you prefer).

You could move the #ifdef inside verbose() to avoid repeating the signature?

Comment on attachment 81250 [details] [review]
[PATCH 5/5] dbus-launch: fix syntax usage

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

Sorry, this is too much code to be worth it. The `eval dbus-launch` form is basically never the best thing to do, particularly now that we have dbus-run-session; I don't want to add more code to support it.

Created attachment 81467 [details] [review]
[PATCH v2 1/4] dbus-launch: fix coding style

Created attachment 81468 [details] [review]
[PATCH v2 2/4] dbus-launch: align document

Created attachment 81470 [details] [review]
[PATCH v2 3/4] dbus-launch: check SIGHUP definition and free args on  failure

I didn't dig into which unix/linux has no SIGHUP, I just add it because it did that in signal_handler. If you're sure it's unnecessary, I'll drop them in v3.

...
static void
signal_handler (int sig)
{
  switch (sig)
    {
#ifdef SIGHUP
    case SIGHUP:
#endif
...

Created attachment 81471 [details] [review]
[PATCH v2 4/4] dbus-launch: do not verbose output if build with  verbose mode disabled

(In reply to comment #13)
> Created attachment 81470 [details] [review] [review]
> [PATCH v2 3/4] dbus-launch: check SIGHUP definition and free args on  failure
> 
> I didn't dig into which unix/linux has no SIGHUP, I just add it because it
> did that in signal_handler. If you're sure it's unnecessary, I'll drop them
> in v3.
> 
> ...
> static void
> signal_handler (int sig)
> {
>   switch (sig)
>     {
> #ifdef SIGHUP
>     case SIGHUP:
> #endif
> ...

Seems it's for windows.

commit 100027007254aaec3ba0388bd0f42e29e512a678
Author: Tor Lillqvist <tml@iki.fi>
Date:   Thu Sep 18 19:40:50 2008 -0400

    [win32] Protect usage of SIGHUP with #ifdef
    
    Signed-off-by: Colin Walters <walters@verbum.org>

(In reply to comment #11)
> [PATCH v2 1/4] dbus-launch: fix coding style

Fixed in git for 1.7.6

(In reply to comment #12)
> [PATCH v2 2/4] dbus-launch: align document

Fixed in git for 1.7.6

(In reply to comment #13)
> free args on  failure

I haven't yet checked the logic for this to make sure it's right. It'd be better as a separate commit, probably.

> I didn't dig into which unix/linux has no SIGHUP, I just add it because it
> did that in signal_handler. If you're sure it's unnecessary, I'll drop them
> in v3.

signal_handler() in dbus-launch.c doesn't need to be portable to Windows: Windows builds use dbus-launch-win.c instead. So, just use SIGHUP unconditionally there.

It looks like unnecessary copy/paste from bus/main.c, which *does* need to be portable to Windows.

(In reply to comment #14)
> [PATCH v2 4/4] dbus-launch: do not verbose output if build with  verbose
> mode disabled

Fixed in git for 1.7.6

Created attachment 81519 [details] [review]
[PATCH v3] dbus-launch: unconditionally use SIGHUP and free memory  on OOM

Fixed in git for 1.7.6, thanks.

Please note in future: our coding style for function calls is "free (x)", not "free(x)".