[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: canonicalize-lgpl bug
From: |
Jim Meyering |
Subject: |
Re: canonicalize-lgpl bug |
Date: |
Wed, 16 Sep 2009 16:48:52 +0200 |
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.
- canonicalize-lgpl bug, Eric Blake, 2009/09/10
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/10
- Re: canonicalize-lgpl bug, Jim Meyering, 2009/09/10
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/10
- Re: canonicalize-lgpl bug, Jim Meyering, 2009/09/11
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/11
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/11
- Re: canonicalize-lgpl bug,
Jim Meyering <=
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/16
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/17
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/17
- Re: canonicalize-lgpl bug, Jim Meyering, 2009/09/17
- Re: canonicalize-lgpl bug, Jim Meyering, 2009/09/18
- Re: canonicalize-lgpl bug, Eric Blake, 2009/09/18