bug-gnulib
[Top][All Lists]
Advanced

[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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]