On 2/19/19 6:38 PM, James Youngman wrote: > Thanks for this fix. > > But, I think the the code I originally I wrote here is broken in a way > that's not completely fixed by your patch. The case is where the > type of a file changes between directory and non-directory. The code > as it is now will issue an error whose message corresponds to ENOTDIR. > If a directory is replaced by an empty file, I think there'a an > argument for returning true without issuing a diagnostic (and hence > without having find return with a nonzero exit status).
This is true: with extreme code like ... int main (int argc, char**argv) { char const *F = "xxx"; while (1) { close (open(F, O_CREAT|O_EXCL|O_CLOEXEC, S_IWUSR|S_IRUSR|S_IRGRP|S_IROTH)); unlink (F); mkdir (F, S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH); rmdir (F); } return 0; } ... running in parallel, I could trigger an error message in every 2nd to 10th execution of 'find'. However, in normal life, I've never seen an issue with -empty yet nor did we receive any bug report about it. I just recognized the FD leaks when reading the code (again), so I fixed that right away. > On the assumption that you also think this race should be handled > silently, how about this approach? > > Instead of keeping the pre-existing race introduced by the iniail > S_ISDIR check, we could begin by open()ing the file, without > O_DIRECTORY. If the open fails, issue an error and return. > Otherwise, perform fstat on the fd. If the fstat fails, issue an > error message and return false. Otherwise, if the file is a regular > file (S_ISREG, as current code) and it is zero bytes long, return > true. Otherwise, if it is a non-directory, return false (as now). > Last, handle the directory case. Perform fdopendir (which, now, we > know cannot fail with ENOTDIR) and attempt readdir on it. If any > readdir returns non-NULL, return false. > > If readdir returns NULL with errno != 0, I think it is probably safest > to issue an error and return false, as now. Except EOVERFLOW: for > this, we should issue no diagnostic and simply return false (since we > know there is something in the directory, even though it cannot be > correctly represented). Otherwise (that is, readdir returns NULL > with errno == 0 and we didn't previously get a non-NULL result) return > true. > > What do you think? Thanks, the idea is nice, however, I see the following problems: a) open()ing also all non-dir files would come with a big performance penalty. As we already have stat()ed the file, we already know if it is empty (or was at that time). I assume that users have already become accustomed to using -empty over "-size 0c", so they'd be wondering about a slowdown. b) open() will already fail if a go-r file is owned by another user. (The same applies to directories, but for these we cannot determine if it is empty anyway.) I think this is a blocker for the idea. As written in the docs, 'find' will always have to fight with races. For me, the question in this -empty case is more whether we should issue an error diagnostic or not. Have a nice day, Berny