fd/1b96912f22390a2a896fc195898e5b5cc76af2

   1 Return-Path: <amdragon@mit.edu>\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 65018431FB6\r
   6         for <notmuch@notmuchmail.org>; Mon,  7 May 2012 15:18:41 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id 3cXfqcv1+zrc for <notmuch@notmuchmail.org>;\r
  16         Mon,  7 May 2012 15:18:40 -0700 (PDT)\r
  17 Received: from dmz-mailsec-scanner-4.mit.edu (DMZ-MAILSEC-SCANNER-4.MIT.EDU\r
  18         [18.9.25.15])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 1F65F431FAE\r
  20         for <notmuch@notmuchmail.org>; Mon,  7 May 2012 15:18:40 -0700 (PDT)\r
  21 X-AuditID: 1209190f-b7f4f6d00000092b-79-4fa84a3dc175\r
  22 Received: from mailhub-auth-2.mit.edu ( [18.7.62.36])\r
  23         by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 38.29.02347.D3A48AF4; Mon,  7 May 2012 18:18:37 -0400 (EDT)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id q47MIZuo013106; \r
  27         Mon, 7 May 2012 18:18:36 -0400\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q47MIXDY005680\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Mon, 7 May 2012 18:18:34 -0400 (EDT)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1SRWGH-0000B2-Hc; Mon, 07 May 2012 18:18:33 -0400\r
  37 Date: Mon, 7 May 2012 18:18:33 -0400\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Jani Nikula <jani@nikula.org>\r
  40 Subject: Re: [PATCH 2/2] new: Centralize file type stat-ing logic\r
  41 Message-ID: <20120507221833.GW2704@mit.edu>\r
  42 References: <1336414186-15293-1-git-send-email-amdragon@mit.edu>\r
  43         <1336414186-15293-3-git-send-email-amdragon@mit.edu>\r
  44         <CAB+hUn_rcz00p4kobjcf57aiyqSAWSPGgRSCNNG0b5eSye=3DQ@mail.gmail.com>\r
  45 MIME-Version: 1.0\r
  46 Content-Type: text/plain; charset=iso-8859-1\r
  47 Content-Disposition: inline\r
  48 Content-Transfer-Encoding: 8bit\r
  49 In-Reply-To:\r
  50  <CAB+hUn_rcz00p4kobjcf57aiyqSAWSPGgRSCNNG0b5eSye=3DQ@mail.gmail.com>\r
  51 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  52 X-Brightmail-Tracker:\r
  53  H4sIAAAAAAAAA+NgFlrCKsWRmVeSWpSXmKPExsUixG6nomvrtcLf4PlOdoum6c4W12/OZLaY\r
  54         cX4XiwOzx637r9k9nq26xeyx7OhPxgDmKC6blNSczLLUIn27BK6MeRO+sRRsdK/49fALUwPj\r
  55         c6MuRk4OCQETiS9bDrND2GISF+6tZwOxhQT2MUqs3MnRxcgFZK9nlGg+t4AdwjnBJLH43DxW\r
  56         CGcJo8Snd90sIC0sAioSk7vesoLYbAIaEtv2L2cEsUUEFCU2n9wPZjML+Eq8vfYArEZYwFFi\r
  57         +pLNzF2MHBy8AtoS/1brQcwE2vy0+RTYGbwCghInZz5hgejVkdi59Q4bSD2zgLTE8n8cEGF5\r
  58         ieats5lBbE6BQInvjzaA2aJA50w5uY1tAqPwLCSTZiGZNAth0iwkkxYwsqxilE3JrdLNTczM\r
  59         KU5N1i1OTszLSy3SNdHLzSzRS00p3cQIigxOSf4djN8OKh1iFOBgVOLhVXyx3F+INbGsuDL3\r
  60         EKMkB5OSKC+z2wp/Ib6k/JTKjMTijPii0pzU4kOMEhzMSiK8bWJAOd6UxMqq1KJ8mJQ0B4uS\r
  61         OK+a1js/IYH0xJLU7NTUgtQimKwMB4eSBO85T6BGwaLU9NSKtMycEoQ0EwcnyHAeoOHeIDW8\r
  62         xQWJucWZ6RD5U4yKUuK8fSAJAZBERmkeXC8scb1iFAd6RZg3D6SKB5j04LpfAQ1mAhq8+dky\r
  63         kMEliQgpqQZGkenxJUfq9s5qqbTf+ZPnXpXAyVnS1sU5/qdLueSOrGsP0REoZHfc2nHF9NRz\r
  64         NcutE1T3PJ6Wu/epfMaGHRlKC1U/vNl6fv+cmjs7Yn2C3nzZ/HWBRzQnY9P5Naz9NeF7p5wx\r
  65         tjK/zcA857/rslZF8T033u55tbFjwsvZm7Q711/gWRRd+nzZXCWW4oxEQy3mouJEAA45T043        AwAA\r
  66 Cc: Notmuch Mail <notmuch@notmuchmail.org>, Vladimir Marek <vlmarek@volny.cz>\r
  67 X-BeenThere: notmuch@notmuchmail.org\r
  68 X-Mailman-Version: 2.1.13\r
  69 Precedence: list\r
  70 List-Id: "Use and development of the notmuch mail system."\r
  71         <notmuch.notmuchmail.org>\r
  72 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  73         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  74 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  75 List-Post: <mailto:notmuch@notmuchmail.org>\r
  76 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  77 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  78         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  79 X-List-Received-Date: Mon, 07 May 2012 22:18:41 -0000\r
  80 \r
  81 Quoth Jani Nikula on May 08 at 12:08 am:\r
  82 >    On May 7, 2012 9:10 PM, "Austin Clements" <[1]amdragon@mit.edu> wrote:\r
  83 >    >\r
  84 >    > This moves our logic to get a file's type into one function.  This has\r
  85 >    > several benefits: we can support OSes and file systems that do not\r
  86 >    > provide dirent.d_type or always return DT_UNKNOWN, complex\r
  87 >    > symlink-handling logic has been replaced by a simple stat fall-through\r
  88 >    > in one place, and the error message for un-stat-able file is more\r
  89 >    > accurate (previously, the error always mentioned directories, even\r
  90 >    > though a broken symlink is not a directory).\r
  91 > \r
  92 >    Please find some quick drive-by-review below.\r
  93 \r
  94 Thanks for the review!  New version on its way...\r
  95 \r
  96 >    J.\r
  97 > \r
  98 >    > ---\r
  99 >    >  notmuch-new.c |   99\r
 100 >    ++++++++++++++++++++++++++++++++++-----------------------\r
 101 >    >  test/new      |    2 +-\r
 102 >    >  2 files changed, 60 insertions(+), 41 deletions(-)\r
 103 >    >\r
 104 >    > diff --git a/notmuch-new.c b/notmuch-new.c\r
 105 >    > index cb720cc..cf2580e 100644\r
 106 >    > --- a/notmuch-new.c\r
 107 >    > +++ b/notmuch-new.c\r
 108 >    > @@ -154,6 +154,44 @@ dirent_sort_strcmp_name (const struct dirent **a,\r
 109 >    const struct dirent **b)\r
 110 >    >     return strcmp ((*a)->d_name, (*b)->d_name);\r
 111 >    >  }\r
 112 >    >\r
 113 >    > +/* Return the type of a directory entry relative to path as a stat(2)\r
 114 >    > + * mode.  Like stat, this follows symlinks.  Returns -1 and sets errno\r
 115 >    > + * if the file's type cannot be determined (which includes dangling\r
 116 >    > + * symlinks).\r
 117 >    > + */\r
 118 >    > +static int\r
 119 >    > +dirent_type (const char *path, const struct dirent *entry)\r
 120 >    > +{\r
 121 >    > +    struct stat statbuf;\r
 122 >    > +    char *abspath;\r
 123 >    > +    int err;\r
 124 >    > +\r
 125 >    > +#ifdef _DIRENT_HAVE_D_TYPE\r
 126 >    > +    /* Mapping from d_type to stat mode_t.  We omit DT_LNK so that\r
 127 >    > +     * we'll fall through to stat and get the real file type. */\r
 128 >    > +    static const mode_t modes[] = {\r
 129 >    > +       [DT_BLK]  = S_IFBLK,\r
 130 >    > +       [DT_CHR]  = S_IFCHR,\r
 131 >    > +       [DT_DIR]  = S_IFDIR,\r
 132 >    > +       [DT_FIFO] = S_IFIFO,\r
 133 >    > +       [DT_REG]  = S_IFREG,\r
 134 >    > +       [DT_SOCK] = S_IFSOCK\r
 135 >    > +    };\r
 136 >    > +    if (entry->d_type < sizeof(modes)/sizeof(modes[0]) &&\r
 137 > \r
 138 >    ARRAY_SIZE()\r
 139 \r
 140 Huh.  I went looking for that.  I wonder how I missed it.\r
 141 \r
 142 >    > +       modes[entry->d_type])\r
 143 >    > +       return modes[entry->d_type];\r
 144 >    > +#endif\r
 145 >    > +\r
 146 >    > +    abspath = talloc_asprintf (NULL, "%s/%s", path, entry->d_name);\r
 147 >    > +    if (!abspath)\r
 148 >    > +       return -1;\r
 149 > \r
 150 >    Does talloc set errno in this case? I suspect not.\r
 151 > \r
 152 >    > +    err = stat(abspath, &statbuf);\r
 153 >    > +    talloc_free (abspath);\r
 154 > \r
 155 >    This likely breaks your promise about errno. You can't trust talloc_free()\r
 156 >    not calling some function that sets errno.\r
 157 \r
 158 Setting errno was a late and apparently not well thought-out addition.\r
 159 The new patch should set it on all of the error paths and save it\r
 160 around things that may modify it.\r
 161 \r
 162 >    > +    if (err < 0)\r
 163 >    > +       return -1;\r
 164 >    > +    return statbuf.st_mode & S_IFMT;\r
 165 >    > +}\r
 166 >    > +\r
 167 >    >  /* Test if the directory looks like a Maildir directory.\r
 168 >    >  *\r
 169 >    >  * Search through the array of directory entries to see if we can find\r
 170 >    all\r
 171 >    > @@ -162,12 +200,12 @@ dirent_sort_strcmp_name (const struct dirent **a,\r
 172 >    const struct dirent **b)\r
 173 >    >  * Return 1 if the directory looks like a Maildir and 0 otherwise.\r
 174 >    >  */\r
 175 >    >  static int\r
 176 >    > -_entries_resemble_maildir (struct dirent **entries, int count)\r
 177 >    > +_entries_resemble_maildir (const char *path, struct dirent **entries,\r
 178 >    int count)\r
 179 >    >  {\r
 180 >    >     int i, found = 0;\r
 181 >    >\r
 182 >    >     for (i = 0; i < count; i++) {\r
 183 >    > -       if (entries[i]->d_type != DT_DIR && entries[i]->d_type !=\r
 184 >    DT_UNKNOWN)\r
 185 >    > +       if (dirent_type (path, entries[i]) != S_IFDIR)\r
 186 >    >            continue;\r
 187 >    >\r
 188 >    >        if (strcmp(entries[i]->d_name, "new") == 0 ||\r
 189 >    > @@ -250,7 +288,7 @@ add_files_recursive (notmuch_database_t *notmuch,\r
 190 >    >     notmuch_status_t status, ret = NOTMUCH_STATUS_SUCCESS;\r
 191 >    >     notmuch_message_t *message = NULL;\r
 192 >    >     struct dirent **fs_entries = NULL;\r
 193 >    > -    int i, num_fs_entries;\r
 194 >    > +    int i, num_fs_entries, entry_type;\r
 195 >    >     notmuch_directory_t *directory;\r
 196 >    >     notmuch_filenames_t *db_files = NULL;\r
 197 >    >     notmuch_filenames_t *db_subdirs = NULL;\r
 198 >    > @@ -317,7 +355,7 @@ add_files_recursive (notmuch_database_t *notmuch,\r
 199 >    >     }\r
 200 >    >\r
 201 >    >     /* Pass 1: Recurse into all sub-directories. */\r
 202 >    > -    is_maildir = _entries_resemble_maildir (fs_entries,\r
 203 >    num_fs_entries);\r
 204 >    > +    is_maildir = _entries_resemble_maildir (path, fs_entries,\r
 205 >    num_fs_entries);\r
 206 >    >\r
 207 >    >     for (i = 0; i < num_fs_entries; i++) {\r
 208 >    >        if (interrupted)\r
 209 >    > @@ -325,17 +363,16 @@ add_files_recursive (notmuch_database_t *notmuch,\r
 210 >    >\r
 211 >    >        entry = fs_entries[i];\r
 212 >    >\r
 213 >    > -       /* We only want to descend into directories.\r
 214 >    > -        * But symlinks can be to directories too, of course.\r
 215 >    > -        *\r
 216 >    > -        * And if the filesystem doesn't tell us the file type in the\r
 217 >    > -        * scandir results, then it might be a directory (and if not,\r
 218 >    > -        * then we'll stat and return immediately in the next level of\r
 219 >    > -        * recursion). */\r
 220 >    > -       if (entry->d_type != DT_DIR &&\r
 221 >    > -           entry->d_type != DT_LNK &&\r
 222 >    > -           entry->d_type != DT_UNKNOWN)\r
 223 >    > -       {\r
 224 >    > +       /* We only want to descend into directories (and symlinks to\r
 225 >    > +        * directories). */\r
 226 >    > +       entry_type = dirent_type (path, entry);\r
 227 >    > +       if (entry_type == -1) {\r
 228 >    > +           /* Be pessimistic, e.g. so we don't loose lots of mail\r
 229 > \r
 230 >    s/loose/lose/ ?\r
 231 \r
 232 Oops.\r
 233 \r
 234 >    > +            * just because a user broke a symlink. */\r
 235 >    > +           fprintf (stderr, "Error reading file %s/%s: %s\n",\r
 236 >    > +                    path, entry->d_name, strerror (errno));\r
 237 > \r
 238 >    You can't trust errno here, as explained above.\r
 239 \r
 240 Fixed above.\r
 241 \r
 242 >    > +           return NOTMUCH_STATUS_FILE_ERROR;\r
 243 >    > +       } else if (entry_type != S_IFDIR) {\r
 244 >    >            continue;\r
 245 >    >        }\r
 246 >    >\r
 247 >    > @@ -425,31 +462,13 @@ add_files_recursive (notmuch_database_t *notmuch,\r
 248 >    >            notmuch_filenames_move_to_next (db_subdirs);\r
 249 >    >        }\r
 250 >    >\r
 251 >    > -       /* If we're looking at a symlink, we only want to add it if it\r
 252 >    > -        * links to a regular file, (and not to a directory, say).\r
 253 >    > -        *\r
 254 >    > -        * Similarly, if the file is of unknown type (due to filesystem\r
 255 >    > -        * limitations), then we also need to look closer.\r
 256 >    > -        *\r
 257 >    > -        * In either case, a stat does the trick.\r
 258 >    > -        */\r
 259 >    > -       if (entry->d_type == DT_LNK || entry->d_type == DT_UNKNOWN) {\r
 260 >    > -           int err;\r
 261 >    > -\r
 262 >    > -           next = talloc_asprintf (notmuch, "%s/%s", path,\r
 263 >    entry->d_name);\r
 264 >    > -           err = stat (next, &st);\r
 265 >    > -           talloc_free (next);\r
 266 >    > -           next = NULL;\r
 267 >    > -\r
 268 >    > -           /* Don't emit an error for a link pointing nowhere, since\r
 269 >    > -            * the directory-traversal pass will have already done\r
 270 >    > -            * that. */\r
 271 >    > -           if (err)\r
 272 >    > -               continue;\r
 273 >    > -\r
 274 >    > -           if (! S_ISREG (st.st_mode))\r
 275 >    > -               continue;\r
 276 >    > -       } else if (entry->d_type != DT_REG) {\r
 277 >    > +       /* Only add regular files (and symlinks to regular files). */\r
 278 >    > +       entry_type = dirent_type (path, entry);\r
 279 >    > +       if (entry_type == -1) {\r
 280 >    > +           fprintf (stderr, "Error reading file %s/%s: %s\n",\r
 281 >    > +                    path, entry->d_name, strerror (errno));\r
 282 > \r
 283 >    Ditto.\r
 284 > \r
 285 >    > +           return NOTMUCH_STATUS_FILE_ERROR;\r
 286 >    > +       } else if (entry_type != S_IFREG) {\r
 287 >    >            continue;\r
 288 >    >        }\r
 289 >    >\r
 290 >    > diff --git a/test/new b/test/new\r
 291 >    > index 26253db..e3900f5 100755\r
 292 >    > --- a/test/new\r
 293 >    > +++ b/test/new\r
 294 >    > @@ -140,7 +140,7 @@ test_begin_subtest "Broken symlink aborts"\r
 295 >    >  ln -s does-not-exist "${MAIL_DIR}/broken"\r
 296 >    >  output=$(NOTMUCH_NEW 2>&1)\r
 297 >    >  test_expect_equal "$output" \\r
 298 >    > -"Error reading directory /run/shm/nm/tmp.new/mail/broken: No such file\r
 299 >    or directory\r
 300 >    > +"Error reading file /run/shm/nm/tmp.new/mail/broken: No such file or\r
 301 >    directory\r
 302 >    >  Note: A fatal error was encountered: Something went wrong trying to\r
 303 >    read or write a file\r
 304 >    >  No new mail."\r
 305 >    >  rm "${MAIL_DIR}/broken"\r
 306 >    >\r
 307 >    > _______________________________________________\r
 308 >    > notmuch mailing list\r
 309 >    > [2]notmuch@notmuchmail.org\r
 310 >    > [3]http://notmuchmail.org/mailman/listinfo/notmuch\r