Re: [PATCH V3 1/2] test/smtp-dummy: add --background option and functionality

[Michal Sojka <sojkam1@fel.cvut.cz>]

Hi Tomi,

On Tue, Sep 18 2012, Tomi Ollila wrote:
> From: Tomi Ollila <too@iki.fi>
>
> When shell executes background process using '&' the scheduling of
> that new process is arbitrary. It could be that smtp-dummy doesn't
> get execution time to listen() it's server socket until some other
> process attempts to connect() to it. The --background option in
> smtp-dummy makes it to go background *after* it started to listen
> its server socket.
>
> When --background option is used, the line "smtp_dummy_pid='<pid>'"
> is printed to stdout from where shell can eval it.
> ---
>
> This is v3 of id:"1323766883-17607-1-git-send-email-tomi.ollila@iki.fi"
>
> addressing (some) Dmitry's comments.
>
>  test/smtp-dummy.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c
> index 86d4316..bb13668 100644
> --- a/test/smtp-dummy.c
> +++ b/test/smtp-dummy.c
> @@ -119,6 +119,7 @@ do_smtp_to_file (FILE *peer, FILE *output)
>  int
>  main (int argc, char *argv[])
>  {
> +	const char * progname;
>  	char *output_filename;
>  	FILE *peer_file, *output;
>  	int sock, peer, err;
> @@ -126,9 +127,31 @@ main (int argc, char *argv[])
>  	struct hostent *hostinfo;
>  	socklen_t peer_addr_len;
>  	int reuse;
> +	int background;
> +
> +	progname = argv[0];
> +
> +	background = 0;
> +	for (; argc >= 2; argc--, argv++) {
> +		if (argv[1][0] != '-')

This is ugly, but all right. I do not think we need to have high
standards for this piece of code :)

> +			break;
> +		if (strcmp (argv[1], "--") == 0) {
> +			argc--;
> +			argv++;
> +			break;
> +		}
> +		if (strcmp (argv[1], "--background") == 0) {
> +			background = 1;
> +			continue;
> +		}
> +		fprintf(stderr, "%s: unregognized option '%s'\n",
> +			progname, argv[1]);
> +		return 1;
> +	}
>
>  	if (argc != 2) {
> -		fprintf (stderr, "Usage: %s <output-file>\n", argv[0]);
> +		fprintf (stderr,
> +			 "Usage: %s [--background] <output-file>\n", progname);
>  		return 1;
>  	}
>
> @@ -181,6 +204,36 @@ main (int argc, char *argv[])
>  		return 1;
>  	}
>
> +	if (background) {
> +		int pid = fork ();
> +		if (pid > 0) {
> +			printf ("smtp_dummy_pid='%d'\n", pid);
> +			fflush (stdout);
> +			close (sock);
> +			return 0;
> +		}
> +		if (pid < 0) {
> +			fprintf (stderr, "Error: fork() failed: %s\n",
> +				 strerror (errno));
> +			close (sock);
> +			return 1;
> +		}
> +		/* Reached if pid == 0 (the child process). */
> +		/* Close stdout so that the one interested in pid value will
> +		   also get EOF. */
> +		close (STDOUT_FILENO);
> +		/* dup2() will re-reserve fd of stdout (1) (opportunistically),
> +		   in case fd of stderr (2) is open. If that was not open we
> +		   don't care fd of stdout (1) either. */
> +		dup2 (STDERR_FILENO, STDOUT_FILENO);
> +
> +		/* This process is now out of reach of shell's job control.
> +		   To resolve the rare but possible condition where this
> +		   "daemon" is started but never connected this process will
> +		   (only) have 30 seconds to exist. */
> +		alarm (30);

30 seconds is probably enough. If it turns out that we need longer
timeout in some scenario, I'd suggest to call alarm() also when
something is received on the socket to extend the timeout.

Overall, both patches LGTM.

-Michal