From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/2] test/smtp-dummy: add --background option for going
	background after listen(2)
In-Reply-To: <1323766883-17607-1-git-send-email-tomi.ollila@iki.fi>
References: <20111212161800.GO2760@mit.edu>
	<1323766883-17607-1-git-send-email-tomi.ollila@iki.fi>
User-Agent: Notmuch/0.10.2+96~g74e5ae5 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 20 Dec 2011 01:28:53 +0400
Message-ID: <874nwwxnca.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Hi Tomi.

On Tue, 13 Dec 2011 11:01:22 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> To avoid the possibility that smtp-dummy doesn't have chance to bind
> its listening socket until something tries to send message to it this
> option makes caller wait until socket is already listening for connections.
> 
> In case this --background option is used, the pid of running smtp-dummy
> is printed on stdout.
> ---
> 
> Resent after whitespace-cleanup in patch 1/2 (this patch).
> 
>  test/smtp-dummy.c |   35 ++++++++++++++++++++++++++++++++++-
>  1 files changed, 34 insertions(+), 1 deletions(-)
> 
> diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c
> index 3801a5e..9126c00 100644
> --- a/test/smtp-dummy.c
> +++ b/test/smtp-dummy.c
> @@ -124,9 +124,21 @@ main (int argc, char *argv[])
>  	struct hostent *hostinfo;
>  	socklen_t peer_addr_len;
>  	int reuse;
> +	int bg;

I would rename bg to background, but that is not important.

> +
> +	/* XXX Quick implementation -- fix if more functionality is desired. */
> +	if (argc >= 2 && strcmp(argv[1], "--background") == 0) {
> +		argc--;
> +		argv[1] = argv[0];
> +		argv++;
> +		bg = 1;
> +	}
> +	else
> +		bg = 0;
>  

Sorry, this code looks ugly and unnecessary complex to me.  I really do
not like messing with argc and argv.  Perhaps something like this would
be better:

  if (argc != 2 && argc != 3)
    usage();
    return 1;

  if (argc > 2) {
    if (argv[1] == background)
      bg = 1;
    else
      usage();
      return 1;
  }

  output = argv[argc - 1];

>  	if (argc != 2) {
> -		fprintf (stderr, "Usage: %s <output-file>\n", argv[0]);
> +		fprintf (stderr, "Usage: %s [--background] <output-file>\n",
> +			 argv[0]);
>  		return 1;
>  	}
>  
> @@ -179,7 +191,27 @@ main (int argc, char *argv[])
>  		return 1;
>  	}
>  
> +	if (bg) {
> +		int pid = fork ();
> +		if (pid > 0) {
> +			printf ("%d\n", pid);
> +			return 0;
> +		}
> +		if (pid < 0) {
> +			fprintf (stderr, "Error: fork() failed: %s\n",
> +				 strerror (errno));
> +			close (sock);
> +			return 1;
> +		}
> +		/* Reached if pid == 0. */
> +		/* Close stdout so that the one interested in pid value will
> +		   also get EOF. */
> +		close (1);

Please use STDOUT_FILENO instead of 1.

> +		/* dup2() will re-reserve fd 1 (opportunistically, in case fd 2
> +		   is open. If that was not open we don't care fd 1 either.) */
> +		dup2 (2, 1);

And STDERR_FILENO and STDOUT_FILENO here.  I would prefer to see "stdout"
and "stderr" instead of "1" and "2" in the comments as well.

Regards,
  Dmitry

> +	}
> +
>  	peer_addr_len = sizeof (peer_addr);
>  	peer = accept (sock, (struct sockaddr *) &peer_addr, &peer_addr_len);
>  	if (peer == -1) {
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch