Re: [PATCH v2 12/13] dir_iterator: new API for iterating over a directory tree
From: Eric Sunshine <hidden>
Date: 2016-06-16 02:19:46
On Fri, Jun 3, 2016 at 8:33 AM, Michael Haggerty [off-list ref] wrote:
The iterator interface is modeled on that for references, though no vtable is necessary because there is (so far?) only one type of dir_iterator. [...]
Some minor comments below, though probably nothing demanding a re-roll...
quoted hunk ↗ jump to hunk
Signed-off-by: Michael Haggerty <redacted> ---diff --git a/dir-iterator.c b/dir-iterator.c@@ -0,0 +1,185 @@ +int dir_iterator_advance(struct dir_iterator *dir_iterator) +{ + struct dir_iterator_int *iter = + (struct dir_iterator_int *)dir_iterator; + + while (1) { + struct dir_iterator_level *level = + &iter->levels[iter->levels_nr - 1]; + struct dirent *de; + + if (!level->initialized) { + if (!is_dir_sep(iter->base.path.buf[iter->base.path.len - 1]))
I realize that dir_iterator_begin() ensures that we never get this far if path has zero length, but that check is so (textually and perhaps mentally) distant from this chunk of code that it's still a little bit scary to see this *potential* access of base.path.buf[-1]. Perhaps an assert(iter->base.path.len) just before this line would document that this condition was taken into consideration?
+ strbuf_addch(&iter->base.path, '/');
+ level->prefix_len = iter->base.path.len;
+
+ /* opendir() errors are handled below */
+ level->dir = opendir(iter->base.path.buf);
+[...]
+ if (!level->dir) {
+ /*
+ * This level is exhausted (or wasn't opened
+ * successfully); pop up a level.
+ */
+ if (--iter->levels_nr == 0) {
+ return dir_iterator_abort(dir_iterator);
+ }Style: unnecessary braces
+ continue;
+ }
+
+ /*
+ * Loop until we find an entry that we can give back
+ * to the caller:
+ */
+ while (1) {
+[...]
+ strbuf_addstr(&iter->base.path, de->d_name);
+ if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
+ if (errno != ENOENT)
+ warning("error reading path '%s': %s",
+ iter->base.path.buf,
+ strerror(errno));Duy's warning_errno() series is already in 'master'...
+ continue; + } + + /* + * We have to set these each time because + * the path strbuf might have been realloc()ed. + */ +
Maybe drop the blank line between the comment and the code to which it applies.
+ iter->base.relative_path =
+ iter->base.path.buf + iter->levels[0].prefix_len;
+ iter->base.basename =
+ iter->base.path.buf + level->prefix_len;
+ level->dir_state = DIR_STATE_ITER;
+
+ return ITER_OK;
+ }
+ }
+}
+
+int dir_iterator_abort(struct dir_iterator *dir_iterator)
+{
+ struct dir_iterator_int *iter = (struct dir_iterator_int *)dir_iterator;
+
+ while (iter->levels_nr) {
+ struct dir_iterator_level *level =
+ &iter->levels[--iter->levels_nr];
It's a bit difficult to locate the loop control embedded within this
(textually) noisy expression. I wonder if it would be easier to read
as a plain for-loop instead:
for (; iter->levels_nr; iter->levels_nr--) {
+ if (level->dir) + closedir(level->dir);
The documentation talks about this function returning ITER_ERROR upon abort failure. This implementation doesn't care about closedir() errors, thus never returns ITER_ERROR, which I guess agrees with dir_iterator_advance() which also doesn't worry about opendir() or closedir() errors. Do opendir() and closedir() errors at least deserve a warning, as lstat() failure does? (Genuine question; I haven't thought too hard about it.)
quoted hunk ↗ jump to hunk
+ } + + free(iter->levels); + strbuf_release(&iter->base.path); + free(iter); + return ITER_DONE; +}diff --git a/dir-iterator.h b/dir-iterator.h@@ -0,0 +1,86 @@ +/* + * Advance the iterator to the first or next item and return ITER_OK. + * If the iteration is exhausted, free the resources associated with + * the iterator and return ITER_DONE. On error, return ITER_ERROR. It + * is a bug to use iterator or call this function again after it has + * returned false. + */ +int dir_iterator_advance(struct dir_iterator *iterator);
The documentation makes it clear that the iterator is freed upon ITER_OK, but I'm having trouble determining if it is freed for ITER_ERROR, as well. The documentation for ref_iterator_advance(), on the other hand, makes it clear that the iterator is freed in all cases.
+/* + * End the iteration before it has been exhausted. Free the directory + * iterator and any associated resources and return ITER_DONE. Return + * ITER_ERROR on error. + */ +int dir_iterator_abort(struct dir_iterator *iterator);