53/f5f6713f9e5b934e42ef5203dda4b5ae606db7

   1 Return-Path: <m.walters@qmul.ac.uk>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 3861D431FB6\r
   6         for <notmuch@notmuchmail.org>; Sun, 18 Nov 2012 07:47:28 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -1.098\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
  12         tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
  13         NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id HKmpMdO4B3lI for <notmuch@notmuchmail.org>;\r
  17         Sun, 18 Nov 2012 07:47:26 -0800 (PST)\r
  18 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
  19         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 43452431FAF\r
  22         for <notmuch@notmuchmail.org>; Sun, 18 Nov 2012 07:47:26 -0800 (PST)\r
  23 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  24         by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
  25         (envelope-from <m.walters@qmul.ac.uk>)\r
  26         id 1Ta75b-0002kx-Di; Sun, 18 Nov 2012 15:47:22 +0000\r
  27 Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost)\r
  28         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
  29         (envelope-from <m.walters@qmul.ac.uk>)\r
  30         id 1Ta75a-0002B3-Vv; Sun, 18 Nov 2012 15:47:19 +0000\r
  31 From: Mark Walters <markwalters1009@gmail.com>\r
  32 To: Peter Wang <novalazy@gmail.com>, notmuch@notmuchmail.org\r
  33 Subject: Re: [PATCH 03/18] insert: open Maildir tmp file\r
  34 In-Reply-To: <1343223767-9812-3-git-send-email-novalazy@gmail.com>\r
  35 References: <1343223767-9812-1-git-send-email-novalazy@gmail.com>\r
  36         <1343223767-9812-3-git-send-email-novalazy@gmail.com>\r
  37 User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1\r
  38         (x86_64-pc-linux-gnu)\r
  39 Date: Sun, 18 Nov 2012 15:47:18 +0000\r
  40 Message-ID: <87wqxiq5fd.fsf@qmul.ac.uk>\r
  41 MIME-Version: 1.0\r
  42 Content-Type: text/plain; charset=us-ascii\r
  43 X-Sender-Host-Address: 93.97.24.31\r
  44 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  45 X-QM-Body-MD5: e9778cad9cbb14edb3cabe66069beb2d (of first 20000 bytes)\r
  46 X-SpamAssassin-Score: -1.8\r
  47 X-SpamAssassin-SpamBar: -\r
  48 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  49         determine if it is\r
  50         spam. We require at least 5.0 points to mark a message as spam.\r
  51         This message scored -1.8 points.\r
  52         Summary of the scoring: \r
  53         * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
  54         *      medium trust\r
  55         *      [138.37.6.40 listed in list.dnswl.org]\r
  56         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  57         provider *      (markwalters1009[at]gmail.com)\r
  58         *  0.5 AWL AWL: From: address is in the auto white-list\r
  59 X-QM-Scan-Virus: ClamAV says the message is clean\r
  60 X-BeenThere: notmuch@notmuchmail.org\r
  61 X-Mailman-Version: 2.1.13\r
  62 Precedence: list\r
  63 List-Id: "Use and development of the notmuch mail system."\r
  64         <notmuch.notmuchmail.org>\r
  65 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  66         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  67 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  68 List-Post: <mailto:notmuch@notmuchmail.org>\r
  69 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  70 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  72 X-List-Received-Date: Sun, 18 Nov 2012 15:47:28 -0000\r
  73 \r
  74 \r
  75 Hi\r
  76 \r
  77 I know this has been around for a long time but it still applies so I\r
  78 will try and review it. I have been using it somewhat and all seems to\r
  79 work.\r
  80 \r
  81 As an aside I have a postpone/resume implementation based on top\r
  82 of it.\r
  83 \r
  84 One general remark is that I think more comments could be helpful.\r
  85 \r
  86 On Wed, 25 Jul 2012, Peter Wang <novalazy@gmail.com> wrote:\r
  87 > Open a unique file in the Maildir tmp directory.\r
  88 > The new message is not yet copied into the file.\r
  89 > ---\r
  90 >  notmuch-insert.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\r
  91 >  1 files changed, 79 insertions(+), 1 deletions(-)\r
  92 >\r
  93 > diff --git a/notmuch-insert.c b/notmuch-insert.c\r
  94 > index 21424cf..f01a6f2 100644\r
  95 > --- a/notmuch-insert.c\r
  96 > +++ b/notmuch-insert.c\r
  97 > @@ -20,12 +20,86 @@\r
  98 >  \r
  99 >  #include "notmuch-client.h"\r
 100 >  \r
 101 > +#include <sys/types.h>\r
 102 > +#include <sys/stat.h>\r
 103 > +#include <fcntl.h>\r
 104 > +\r
 105 > +static notmuch_bool_t\r
 106 > +safe_gethostname (char *hostname, size_t hostname_size)\r
 107 > +{\r
 108 > +    if (gethostname (hostname, hostname_size) == -1) {\r
 109 > +     strncpy (hostname, "unknown", hostname_size);\r
 110 > +    }\r
 111 > +    hostname[hostname_size - 1] = '\0';\r
 112 > +\r
 113 > +    return (strchr (hostname, '/') == NULL);\r
 114 > +}\r
 115 \r
 116 Here is an example: a comment saying "like gethostname but guarantees\r
 117 that hostname is  null terminated. Returns true unless hostname contains\r
 118 a /.  \r
 119 \r
 120 Also I found hostname_size a confusing term as it the size of the buffer\r
 121 which is allowed to contain hostname rather than the size of\r
 122 hostname. Maybe hostname_buffer_size or just len?\r
 123 \r
 124 > +\r
 125 > +static int\r
 126 > +maildir_open_tmp (void *ctx, const char *dir, char **tmppath, char **newpath)\r
 127 > +{\r
 128 > +    pid_t pid;\r
 129 > +    char hostname[256];\r
 130 > +    struct timeval tv;\r
 131 > +    char *filename;\r
 132 > +    int fd = -1;\r
 133 > +\r
 134 > +    /* This is the file name format used by Dovecot. */\r
 135 > +    pid = getpid ();\r
 136 > +    if (! safe_gethostname (hostname, sizeof (hostname))) {\r
 137 > +     fprintf (stderr, "Error: invalid host name.\n");\r
 138 > +     return -1;\r
 139 > +    }\r
 140 > +    gettimeofday (&tv, NULL);\r
 141 > +    filename = talloc_asprintf (ctx, "%ld.M%ldP%d.%s",\r
 142 > +                             tv.tv_sec, tv.tv_usec, pid, hostname);\r
 143 > +\r
 144 > +    *tmppath = talloc_asprintf (ctx, "%s/tmp/%s", dir, filename);\r
 145 \r
 146 Does there need to be rather more error checking after talloc\r
 147 operations? Eg what if this allocation fails?\r
 148 \r
 149 > +\r
 150 > +    do {\r
 151 > +     fd = open (*tmppath, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, 0666);\r
 152 > +    } while (fd == -1 && errno == EEXIST);\r
 153 \r
 154 Am I misunderstanding or does this deadlock if the file *tmppath exists?\r
 155 Do you want to recalculate tmppath inside the do while loop?\r
 156 \r
 157 Also is 0666 standard or would 0644 or 0600 be better?\r
 158 \r
 159 Best wishes\r
 160 \r
 161 Mark\r
 162 \r
 163 > +\r
 164 > +    if (fd != -1) {\r
 165 > +     *newpath = talloc_asprintf (ctx, "%s/new/%s", dir, filename);\r
 166 > +    }\r
 167 > +    else {\r
 168 > +     fprintf (stderr, "Error: opening %s: %s\n",\r
 169 > +              *tmppath, strerror (errno));\r
 170 > +     talloc_free (*tmppath);\r
 171 > +    }\r
 172 > +\r
 173 > +    talloc_free (filename);\r
 174 > +\r
 175 > +    return fd;\r
 176 > +}\r
 177 > +\r
 178 > +static notmuch_bool_t\r
 179 > +insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,\r
 180 > +             const char *dir)\r
 181 > +{\r
 182 > +    char *tmppath;\r
 183 > +    char *newpath;\r
 184 > +    int fdout;\r
 185 > +\r
 186 > +    fdout = maildir_open_tmp (ctx, dir, &tmppath, &newpath);\r
 187 > +    if (fdout < 0) {\r
 188 > +     return FALSE;\r
 189 > +    }\r
 190 > +\r
 191 > +    close (fdout);\r
 192 > +    unlink (tmppath);\r
 193 > +    return FALSE;\r
 194 > +}\r
 195 > +\r
 196 >  int\r
 197 >  notmuch_insert_command (void *ctx, int argc, char *argv[])\r
 198 >  {\r
 199 >      notmuch_config_t *config;\r
 200 >      notmuch_database_t *notmuch;\r
 201 >      const char *db_path;\r
 202 > +    char *maildir;\r
 203 > +    notmuch_bool_t ret;\r
 204 >  \r
 205 >      config = notmuch_config_open (ctx, NULL, NULL);\r
 206 >      if (config == NULL)\r
 207 > @@ -33,11 +107,15 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])\r
 208 >  \r
 209 >      db_path = notmuch_config_get_database_path (config);\r
 210 >  \r
 211 > +    maildir = talloc_asprintf (ctx, "%s", db_path);\r
 212 > +\r
 213 >      if (notmuch_database_open (notmuch_config_get_database_path (config),\r
 214 >                              NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))\r
 215 >       return 1;\r
 216 >  \r
 217 > +    ret = insert_message (ctx, notmuch, STDIN_FILENO, maildir);\r
 218 > +\r
 219 >      notmuch_database_destroy (notmuch);\r
 220 >  \r
 221 > -    return 1;\r
 222 > +    return (ret) ? 0 : 1;\r
 223 >  }\r
 224 > -- \r
 225 > 1.7.4.4\r
 226 >\r
 227 > _______________________________________________\r
 228 > notmuch mailing list\r
 229 > notmuch@notmuchmail.org\r
 230 > http://notmuchmail.org/mailman/listinfo/notmuch\r