From: Mark Walters <markwalters1009@gmail.com>
To: Peter Wang <novalazy@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 03/18] insert: open Maildir tmp file
In-Reply-To: <1343223767-9812-3-git-send-email-novalazy@gmail.com>
References: <1343223767-9812-1-git-send-email-novalazy@gmail.com>
	<1343223767-9812-3-git-send-email-novalazy@gmail.com>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sun, 18 Nov 2012 15:47:18 +0000
Message-ID: <87wqxiq5fd.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi

I know this has been around for a long time but it still applies so I
will try and review it. I have been using it somewhat and all seems to
work.

As an aside I have a postpone/resume implementation based on top
of it.

One general remark is that I think more comments could be helpful.

On Wed, 25 Jul 2012, Peter Wang <novalazy@gmail.com> wrote:
> Open a unique file in the Maildir tmp directory.
> The new message is not yet copied into the file.
> ---
>  notmuch-insert.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 79 insertions(+), 1 deletions(-)
>
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 21424cf..f01a6f2 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -20,12 +20,86 @@
>  
>  #include "notmuch-client.h"
>  
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
> +static notmuch_bool_t
> +safe_gethostname (char *hostname, size_t hostname_size)
> +{
> +    if (gethostname (hostname, hostname_size) == -1) {
> +	strncpy (hostname, "unknown", hostname_size);
> +    }
> +    hostname[hostname_size - 1] = '\0';
> +
> +    return (strchr (hostname, '/') == NULL);
> +}

Here is an example: a comment saying "like gethostname but guarantees
that hostname is  null terminated. Returns true unless hostname contains
a /.  

Also I found hostname_size a confusing term as it the size of the buffer
which is allowed to contain hostname rather than the size of
hostname. Maybe hostname_buffer_size or just len?

> +
> +static int
> +maildir_open_tmp (void *ctx, const char *dir, char **tmppath, char **newpath)
> +{
> +    pid_t pid;
> +    char hostname[256];
> +    struct timeval tv;
> +    char *filename;
> +    int fd = -1;
> +
> +    /* This is the file name format used by Dovecot. */
> +    pid = getpid ();
> +    if (! safe_gethostname (hostname, sizeof (hostname))) {
> +	fprintf (stderr, "Error: invalid host name.\n");
> +	return -1;
> +    }
> +    gettimeofday (&tv, NULL);
> +    filename = talloc_asprintf (ctx, "%ld.M%ldP%d.%s",
> +				tv.tv_sec, tv.tv_usec, pid, hostname);
> +
> +    *tmppath = talloc_asprintf (ctx, "%s/tmp/%s", dir, filename);

Does there need to be rather more error checking after talloc
operations? Eg what if this allocation fails?

> +
> +    do {
> +	fd = open (*tmppath, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, 0666);
> +    } while (fd == -1 && errno == EEXIST);

Am I misunderstanding or does this deadlock if the file *tmppath exists?
Do you want to recalculate tmppath inside the do while loop?

Also is 0666 standard or would 0644 or 0600 be better?

Best wishes

Mark

> +
> +    if (fd != -1) {
> +	*newpath = talloc_asprintf (ctx, "%s/new/%s", dir, filename);
> +    }
> +    else {
> +	fprintf (stderr, "Error: opening %s: %s\n",
> +		 *tmppath, strerror (errno));
> +	talloc_free (*tmppath);
> +    }
> +
> +    talloc_free (filename);
> +
> +    return fd;
> +}
> +
> +static notmuch_bool_t
> +insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
> +		const char *dir)
> +{
> +    char *tmppath;
> +    char *newpath;
> +    int fdout;
> +
> +    fdout = maildir_open_tmp (ctx, dir, &tmppath, &newpath);
> +    if (fdout < 0) {
> +	return FALSE;
> +    }
> +
> +    close (fdout);
> +    unlink (tmppath);
> +    return FALSE;
> +}
> +
>  int
>  notmuch_insert_command (void *ctx, int argc, char *argv[])
>  {
>      notmuch_config_t *config;
>      notmuch_database_t *notmuch;
>      const char *db_path;
> +    char *maildir;
> +    notmuch_bool_t ret;
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
>      if (config == NULL)
> @@ -33,11 +107,15 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
>  
>      db_path = notmuch_config_get_database_path (config);
>  
> +    maildir = talloc_asprintf (ctx, "%s", db_path);
> +
>      if (notmuch_database_open (notmuch_config_get_database_path (config),
>  			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
>  	return 1;
>  
> +    ret = insert_message (ctx, notmuch, STDIN_FILENO, maildir);
> +
>      notmuch_database_destroy (notmuch);
>  
> -    return 1;
> +    return (ret) ? 0 : 1;
>  }
> -- 
> 1.7.4.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch