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

[David Turner <dturner@twopensource.com>]

On Fri, 2016-04-15 at 17:07 -0700, Stefan Beller wrote:
> > +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:
> > 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.