Eric Blake wrote: > Jim Meyering <jim <at> meyering.net> writes: >> > Eric Blake (6): >> > canonicalize-lgpl: reject non-directory with trailing slash >> > stdlib: sort witness names >> > canonicalize: leave canonicalize_file_name to canonicalize-lgpl >> > canonicalize-lgpl: use native realpath if it works >> > canonicalize: don't lose errno >> > canonicalize: honor // if distinct from / >> >> Sounds promising. Thanks! >> >> I'll read the above more carefully >> and take a look at the changes on Monday or Tuesday. > > Here's the improved, refactored series, with some review commentary. c_f_n = > canonicalize_file_name, c_f_mode = canonicalize_filename_mode, Because of the > glibc 2.3.5 bug in realpath/c_f_n, it still makes sense for canonicalize to > provide a working c_f_n replacement, so I dropped patch 3 from the above > series. Of the three interfaces, only realpath is specified by POSIX, but > POSIX admits that it is useless without a NULL second argument, at which point > it is easier to read c_f_n(name) than realpath(name,NULL). As a result, if > you > import: > > canonicalize only => GPL c_f_n, GPL c_f_mode > canonicalize-lgpl only => LGPL realpath, LGPL c_f_n > both modules => LGPL realpath, LGPL c_f_n, GPL c_f_mode > > Eric Blake (11): > [1/11] stdlib: sort witness names > cleanup done up front, but could be delayed until just before patch 7 > > [2/11] canonicalize, canonicalize-lgpl: update module dependencies > we were checking whether c_f_n name was declared, but without turning on > extensions (defeating the purpose, since glibc is the only platform that > provides it, but hides it behind extensions). By adding extensions, we can > drop the decl check altogether. Also, prior to this patch, using just the > module pair 'canonicalize-lgpl sys_stat' failed to compile on mingw, due to a > link failure on lstat. > > [3/11] canonicalize: don't lose errno > glibc still has a bug in realpath/c_f_n where errno could be inadvertently > changed by a call to free() during an error return, but canonicalize-lgpl was > immune, and now canonicalize is fixed. I guess I'll have to file a glibc bug > report for the gnulib->glibc syncing (patch 9 gets the glibc->gnulib syncing)
To ease future glibc/gnulib syncing, it'd be better not to change __set_errno (...) to errno = ... No? Also, if you do make a mechanical change like that, it's easier on reviewers and general maintenance to keep that in a change-set separate from any delta that makes a semantic change, like your "don't lose errno" fix does. > [4/11] canonicalize: avoid resolvepath > using resolvepath made sense for Solaris back when all this module did was > replace c_f_n (and before canonicalize-lgpl did the same replacement, but with > LGPL). But c_f_mode can't exploit resolvepath, so our c_f_n implementation > was > just extra bulk on Solaris > > [5/11] test-canonicalize-lgpl: consolidate into single C program > [6/11] test-canonicalize: consolidate into single C program > 5 and 6 could be squashed together, if desired. I got tired of having to > clean > up leftovers and restart an entire testsuite when debugging triggering > failures. By moving all the symlink creation and cleanup into C, the test app > now works standalone from the debugger. As a side effect, also avoids the > fact > that prior to this point, both test-canonicalize.sh and test-canonicalize- > lgpl.sh tried to create the same symlink "ise", wreaking havoc on poorly timed > parallel tests. This also reorders the tests slightly; on mingw, the overall > test still exits with status 77 (skip), but at least it did real tests on non- > symlinks before that point. I've reached this point in reading the patches. So far they look fine. I will read the remainder, and test tomorrow. BTW, I appreciate these commentaries. Have you considered adding something like that to each commit log? IMHO, this sort of justification/summary is worth at least as much as the ChangeLog-style entries.
