Re: [PATCH v4 09/16] index-helper: use watchman to avoid refreshing index with lstat()

On Fri, 2016-04-15 at 17:07 -0700, Stefan Beller wrote:

quoted

+static void refresh_by_watchman(struct index_state *istate)
+{
+       void *shm = NULL;
+       int length;
+       int i;
+       struct stat st;
+       int fd = -1;
+       const char *path = index_helper_path("git-watchman-%s
-%"PRIuMAX,
+                                            sha1_to_hex(istate
->sha1),
+                                            (uintmax_t)getpid());
+
+       fd = open(path, O_RDONLY);
+       if (fd < 0)
+               return;
+
+       /*
+        * This watchman data is just for us -- no need to keep it
+        * around once we've got it open.
+        */
+       unlink(path);
+
+       if (fstat(fd, &st) < 0)
+               goto done;
+
+       length = st.st_size;
+       shm = mmap(NULL, length, PROT_READ, MAP_SHARED, fd, 0);
+
+       if (shm == MAP_FAILED)
+               goto done;
+
+       close(fd);
+       fd = -1;
+
+       if (length <= 20 ||
+           hashcmp(istate->sha1, (unsigned char *)shm + length -
20) ||
+           /*
+            * No need to clear CE_WATCHMAN_DIRTY set by 'WAMA' on
+            * disk. Watchman can only set more, not clear any, so
+            * this is OR mask.
+            */
+           read_watchman_ext(istate, shm, length - 20))
+               goto done;
+
+       /*
+        * Now that we've marked the invalid entries in the
+        * untracked-cache itself, we can erase them from the list
of
+        * entries to be processed and mark the untracked cache for
+        * watchman usage.
+        */
+       if (istate->untracked) {
+               string_list_clear(&istate->untracked
->invalid_untracked, 0);
+               istate->untracked->use_watchman = 1;
+       }
+
+       for (i = 0; i < istate->cache_nr; i++) {
+               struct cache_entry *ce = istate->cache[i];
+               if (ce_stage(ce) || (ce->ce_flags &
CE_WATCHMAN_DIRTY))
+                       continue;
+               ce_mark_uptodate(ce);
+       }
+done:
+       if (shm)
+               munmap(shm, length);
+
+       if (fd > 0)
+               close(fd);

coverities opinion:

quoted

off_by_one: Testing whether handle fd is strictly greater than zero
is suspicious.
fd leaks when it is zero. Did you intend to include equality with
zero?

We can assert that fd is never 0, because that is where stdin is?

You are right: it should be if (fd >= 0).  Will fix. Thanks.