[PATCH v2 05/20] insert: copy stdin to Maildir tmp file

[Tomi Ollila]

On Sun, Nov 25 2012, Peter Wang <novalazy at gmail.com> wrote:

> Read the new message from standard input into the Maildir tmp file.
> ---

There are a few issues that gort my attention in this particular function:


>  notmuch-insert.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 371fb47..88e8533 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -94,6 +94,47 @@ maildir_open_tmp_file (void *ctx, const char *dir,
>      return fd;
>  }
>  
> +/* Copy the contents of fdin into fdout. */
> +static notmuch_bool_t
> +copy_fd_data (int fdin, int fdout)
> +{
> +    char buf[4096];

Copying in 4k blocks is slow when at least when doing file to file copy.
Also socket buffers can often hold much more data. When reading from
network and saving to file (in low-load machine) this is OK, but otherwise
something like 64k buffer works better(*).

(*) Now that I said it I have to measure this yet another time ;)

> +    char *p;
> +    ssize_t remain;
> +    ssize_t written;
> +
> +    for (;;) {
> +	remain = read (fdin, buf, sizeof(buf));

space between sizeof and (buf)

> +	if (remain == 0)
> +	    break;
> +	if (remain < 0) {
> +	    if (errno == EINTR)
> +		continue;
> +	    fprintf (stderr, "Error: reading from standard input: %s\n",
> +		     strerror (errno));
> +	    return FALSE;
> +	}

You're claiming in function name & and its description that this is more
"generic" copy function -- yet error message speaks about 'standard input'.

> +
> +	p = buf;
> +	do {
> +	    written = write (fdout, p, remain);
> +	    if (written == 0)
> +		return FALSE;
> +	    if (written < 0) {
> +		if (errno == EINTR)
> +		    continue;
> +		fprintf (stderr, "Error: writing to temporary file: %s",
> +			 strerror (errno));
> +		return FALSE;
> +	    }

Ditto.

> +	    p += written;
> +	    remain -= written;
> +	} while (remain > 0);
> +    }
> +
> +    return TRUE;
> +}
> +

Tomi