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, ¬much)) > 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