Re: [patch] utils/path.cc fixes and testsuite
On Mar 8 22:24, Christopher Faylor wrote: On Sat, Mar 08, 2008 at 07:10:03PM -0800, Brian Dessent wrote: Index: Makefile.in === RCS file: /cvs/src/src/winsup/utils/Makefile.in,v retrieving revision 1.69 diff -u -p -r1.69 Makefile.in --- Makefile.in 8 Mar 2008 17:52:49 - 1.69 +++ Makefile.in 9 Mar 2008 03:01:17 - @@ -99,10 +99,23 @@ else all: warn_cygcheck_zlib endif -# the rest of this file contains generic rules - all: Makefile $(CYGWIN_BINS) $(MINGW_BINS) +# test harness support (note: the MINGW_BINS += should come after the +# all: above so that the testsuite is not run for make but only +# make check.) +MINGW_BINS += testsuite.exe Doesn't that install testsuite.exe at `make install' time? Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
Re: [patch] utils/path.cc fixes and testsuite
Corinna Vinschen wrote: Doesn't that install testsuite.exe at `make install' time? Ack, how about the attached? Brian2008-03-09 Brian Dessent [EMAIL PROTECTED] * Makefile.in (install): Don't install the testsuite. Index: Makefile.in === RCS file: /cvs/src/src/winsup/utils/Makefile.in,v retrieving revision 1.70 diff -u -p -r1.70 Makefile.in --- Makefile.in 9 Mar 2008 04:10:10 - 1.70 +++ Makefile.in 9 Mar 2008 08:52:06 - @@ -157,7 +157,7 @@ realclean: clean install: all $(SHELL) $(updir1)/mkinstalldirs $(bindir) - for i in $(CYGWIN_BINS) $(MINGW_BINS) ; do \ + for i in $(CYGWIN_BINS) ${filter-out testsuite.exe,$(MINGW_BINS)} ; do \ n=`echo $$i | sed '$(program_transform_name)'`; \ $(INSTALL_PROGRAM) $$i $(bindir)/$$n; \ done
Re: [patch] utils/path.cc fixes and testsuite
On Mar 9 00:55, Brian Dessent wrote: Corinna Vinschen wrote: Doesn't that install testsuite.exe at `make install' time? Ack, how about the attached? Fine with me. Thanks. Corinna
Re: [patch] utils/path.cc fixes and testsuite
On Sun, Mar 09, 2008 at 12:55:13AM -0800, Brian Dessent wrote: Corinna Vinschen wrote: Doesn't that install testsuite.exe at `make install' time? Ack, how about the attached? Brian 2008-03-09 Brian Dessent [EMAIL PROTECTED] * Makefile.in (install): Don't install the testsuite. Index: Makefile.in === RCS file: /cvs/src/src/winsup/utils/Makefile.in,v retrieving revision 1.70 diff -u -p -r1.70 Makefile.in --- Makefile.in9 Mar 2008 04:10:10 - 1.70 +++ Makefile.in9 Mar 2008 08:52:06 - @@ -157,7 +157,7 @@ realclean: clean install: all $(SHELL) $(updir1)/mkinstalldirs $(bindir) - for i in $(CYGWIN_BINS) $(MINGW_BINS) ; do \ + for i in $(CYGWIN_BINS) ${filter-out testsuite.exe,$(MINGW_BINS)} ; do \ n=`echo $$i | sed '$(program_transform_name)'`; \ $(INSTALL_PROGRAM) $$i $(bindir)/$$n; \ done Out of curiousity, does anyone know if program_transform_name ever gets set to anything for cygwin? If not, this loop could be made into a make construct. I suspect that it probably is used to translate cygcheck.exe - i686-pc-cygwin-cygcheck, even though that would be worthless. You probably could get rid of it even if it did get set but it would be trickier. cgf
[patch] utils/path.cc fixes and testsuite
The winsup/utils/path.cc file implements a primative POSIX-Win32 path conversion API that is independant of the real one in Cygwin. This is used by cygcheck as well as strace (for opening the -o parameter). Currently this code has bitrotted a bit, and its handling of relative paths seems to be a little awkward. Examples of how it's broken: strace -o foo.out cmd will place foo.out file in / (or some other strange location, usually the root of whatever prefix of the mount table matched CWD) instead of the CWD. cd /bin cygcheck ls reports Error: could not find ls, but cd /bin cygcheck ./ls does work. cd / cygcheck usr/bin/ls reports usr/bin/ls - Cannot open but cd / cygcheck bin/ls works. cp /bin/ls /tmp cd / cygcheck tmp/ls reports tmp/ls - Cannot open. Anyway, I took a look at fixing this so that the path.cc code is a little more robust. But as I'm sure you're aware, messing with path conversion code is really annoying when you factor in all sorts of weird corner cases, so I decided to make a testsuite for this code so that it would be possible to both cure the bitrot as well as know if it has regressed again. That attached patch does both. The harness that I came up with consists of simply a header (testsuite.h) which contains definitions for both a toy mount table as well as a series of {CWD,POSIX input,expected Win32 output} tuples that form the tests. The driver is testsuite.c, and it works by recompiling path.cc with -DTESTSUITE to activate the hooks. It's pretty simplistic, but it has allowed me to fix a number of corner cases in the code such that all of the above mentioned oddities are now gone. In order to make the testsuite pass I also had to add a little bit of normalization code so that e.g. the Win32 output always uses backslashes and that it doesn't leave repeated slashes between parts, e.g. foo\\/bar. For the sample mount table I tried to emulate what a real mount table is typically like, with a couple additional mounts. The Makefile changes to support this are pretty straightforward: nothing changes for the default make all case. If you run make check it builds the required files and runs the testsuite. I've attached a sample output. Brian2008-03-08 Brian Dessent [EMAIL PROTECTED] * Makefile.in: Add a 'check' target that builds and runs testsuite.exe from path-testsuite.o and testsuite.o. * path.cc: Include testsuite.h. (struct mnt): Change to a mnt_t typedef and don't define mount_table when TESTSUITE is defined. (find2): Don't include when TESTSUITE is defined to avoid warning. (get_cygdrive0): Ditto. (get_cygdrive): Ditto. (read_mounts): Provide empty implementation when TESTSUITE is defined. (vconcat): Use the isslash macro. (unconvert_slashes): New helper to convert to backslashses. (rel_vconcat): Handle relative paths more gracefully. (cygpath): Skip a leading ./ sequence. Avoid double-slashes. Normalize final output to backslashes and remove redundant path sequences. * testsuite.cc: New file implementing testsuite driver. * testsuite.h: New header implementing harness mount table and series of tests. Makefile.in | 19 +++- path.cc | 57 + testsuite.cc | 89 testsuite.h | 130 +++ 4 files changed, 283 insertions(+), 12 deletions(-) Index: Makefile.in === RCS file: /cvs/src/src/winsup/utils/Makefile.in,v retrieving revision 1.69 diff -u -p -r1.69 Makefile.in --- Makefile.in 8 Mar 2008 17:52:49 - 1.69 +++ Makefile.in 8 Mar 2008 18:11:10 - @@ -99,10 +99,23 @@ else all: warn_cygcheck_zlib endif -# the rest of this file contains generic rules - all: Makefile $(CYGWIN_BINS) $(MINGW_BINS) +# test harness support (note: the MINGW_BINS += should come after the +# all: above so that the testsuite is not run for make but only +# make check.) +MINGW_BINS += testsuite.exe +MINGW_OBJS += path-testsuite.o testsuite.o +testsuite.exe: path-testsuite.o +path-testsuite.cc: path.cc ; ln -sf ${filter %.cc,$^} $@ +path-testsuite.o: MINGW_CXXFLAGS += -DTESTSUITE +# this is necessary because this .c lives in the build dir instead of src +path-testsuite.o: MINGW_CXX := ${patsubst -I.,-I$(utils_source),$(MINGW_CXX)} +path-testsuite.cc path.cc testsuite.cc: testsuite.h +check: testsuite.exe ; $(D)/$(F) + +# the rest of this file contains generic rules + # how to compile a MinGW object $(MINGW_OBJS): %.o: %.cc ifdef VERBOSE @@ -137,7 +150,7 @@ $(MINGW_BINS): $(MINGW_DEP_LDLIBS) $(CYGWIN_BINS): $(ALL_DEP_LDLIBS) clean: - rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) + rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) path-testsuite.cc testsuite.exe realclean: clean rm -f Makefile config.cache Index:
Re: [patch] utils/path.cc fixes and testsuite
On Sat, Mar 08, 2008 at 11:01:32AM -0800, Brian Dessent wrote: The winsup/utils/path.cc file implements a primative POSIX-Win32 path conversion API that is independant of the real one in Cygwin. This is used by cygcheck as well as strace (for opening the -o parameter). Currently this code has bitrotted a bit, and its handling of relative paths seems to be a little awkward. Examples of how it's broken: strace -o foo.out cmd will place foo.out file in / (or some other strange location, usually the root of whatever prefix of the mount table matched CWD) instead of the CWD. cd /bin cygcheck ls reports Error: could not find ls, but cd /bin cygcheck ./ls does work. cd / cygcheck usr/bin/ls reports usr/bin/ls - Cannot open but cd / cygcheck bin/ls works. cp /bin/ls /tmp cd / cygcheck tmp/ls reports tmp/ls - Cannot open. Anyway, I took a look at fixing this so that the path.cc code is a little more robust. But as I'm sure you're aware, messing with path conversion code is really annoying when you factor in all sorts of weird corner cases, so I decided to make a testsuite for this code so that it would be possible to both cure the bitrot as well as know if it has regressed again. That attached patch does both. The harness that I came up with consists of simply a header (testsuite.h) which contains definitions for both a toy mount table as well as a series of {CWD,POSIX input,expected Win32 output} tuples that form the tests. The driver is testsuite.c, and it works by recompiling path.cc with -DTESTSUITE to activate the hooks. It's pretty simplistic, but it has allowed me to fix a number of corner cases in the code such that all of the above mentioned oddities are now gone. In order to make the testsuite pass I also had to add a little bit of normalization code so that e.g. the Win32 output always uses backslashes and that it doesn't leave repeated slashes between parts, e.g. foo\\/bar. For the sample mount table I tried to emulate what a real mount table is typically like, with a couple additional mounts. The Makefile changes to support this are pretty straightforward: nothing changes for the default make all case. If you run make check it builds the required files and runs the testsuite. I've attached a sample output. Brian 2008-03-08 Brian Dessent [EMAIL PROTECTED] * Makefile.in: Add a 'check' target that builds and runs testsuite.exe from path-testsuite.o and testsuite.o. * path.cc: Include testsuite.h. (struct mnt): Change to a mnt_t typedef and don't define mount_table when TESTSUITE is defined. (find2): Don't include when TESTSUITE is defined to avoid warning. (get_cygdrive0): Ditto. (get_cygdrive): Ditto. (read_mounts): Provide empty implementation when TESTSUITE is defined. (vconcat): Use the isslash macro. (unconvert_slashes): New helper to convert to backslashses. (rel_vconcat): Handle relative paths more gracefully. (cygpath): Skip a leading ./ sequence. Avoid double-slashes. Normalize final output to backslashes and remove redundant path sequences. * testsuite.cc: New file implementing testsuite driver. * testsuite.h: New header implementing harness mount table and series of tests. Makefile.in | 19 +++- path.cc | 57 + testsuite.cc | 89 testsuite.h | 130 +++ 4 files changed, 283 insertions(+), 12 deletions(-) Index: Makefile.in === RCS file: /cvs/src/src/winsup/utils/Makefile.in,v retrieving revision 1.69 diff -u -p -r1.69 Makefile.in --- Makefile.in8 Mar 2008 17:52:49 - 1.69 +++ Makefile.in8 Mar 2008 18:11:10 - @@ -99,10 +99,23 @@ else all: warn_cygcheck_zlib endif -# the rest of this file contains generic rules - all: Makefile $(CYGWIN_BINS) $(MINGW_BINS) +# test harness support (note: the MINGW_BINS += should come after the +# all: above so that the testsuite is not run for make but only +# make check.) +MINGW_BINS += testsuite.exe +MINGW_OBJS += path-testsuite.o testsuite.o +testsuite.exe: path-testsuite.o +path-testsuite.cc: path.cc ; ln -sf ${filter %.cc,$^} $@ +path-testsuite.o: MINGW_CXXFLAGS += -DTESTSUITE +# this is necessary because this .c lives in the build dir instead of src +path-testsuite.o: MINGW_CXX := ${patsubst -I.,-I$(utils_source),$(MINGW_CXX)} +path-testsuite.cc path.cc testsuite.cc: testsuite.h +check: testsuite.exe ; $(D)/$(F) + +# the rest of this file contains generic rules + # how to compile a MinGW object $(MINGW_OBJS): %.o: %.cc ifdef VERBOSE @@ -137,7 +150,7 @@ $(MINGW_BINS): $(MINGW_DEP_LDLIBS) $(CYGWIN_BINS): $(ALL_DEP_LDLIBS) clean: - rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) + rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) path-testsuite.cc testsuite.exe
Re: [patch] utils/path.cc fixes and testsuite
Christopher Faylor wrote: Would it be possible to uncollapse this if block and make it a little clearer? The else nine lines away makes it a little hard to follow. So, wouldn't just inverting the if (match) to if (!match) and putting the else condition cause some unnesting? Here's a v2 patch. Changes: - As suggested I used path[max_len] instead of *(path + max_len). I had done it that way originally to try to make it clear that I was looking at what the first chararacter of the path + max_len argument to concat, but I now agree it's kind of unidiomatic C. - Rearranged the if/else block, and added a comment for the logic of each branch. - Added a test for UNC paths. - Minor tweak on the Makefile rule that symlinks the source file to another name to prevent it from running every time. In general I'm not all that crazy with this idea of symlinking a file in order to compile it to a differently-named object, but doing it otherwise would require repeating the compile rule with all its ugly VERBOSE casing and I just went to the trouble to eliminate all such repetition in the Makefile. Brian2008-03-08 Brian Dessent [EMAIL PROTECTED] * Makefile.in: Add a 'check' target that builds and runs testsuite.exe from path-testsuite.o and testsuite.o. * path.cc: Include testsuite.h. (struct mnt): Change to a mnt_t typedef and don't define mount_table when TESTSUITE is defined. (find2): Don't include when TESTSUITE is defined to avoid warning. (get_cygdrive0): Ditto. (get_cygdrive): Ditto. (read_mounts): Provide empty implementation when TESTSUITE is defined. (vconcat): Use the isslash macro. (unconvert_slashes): New helper to convert to backslashses. (rel_vconcat): Handle relative paths more gracefully. (cygpath): Skip a leading ./ sequence. Avoid double-slashes. Normalize final output to backslashes and remove redundant path sequences. * testsuite.cc: New file implementing testsuite driver. * testsuite.h: New header implementing harness mount table and series of tests. Makefile.in | 19 +++- path.cc | 71 +++ testsuite.cc | 89 testsuite.h | 131 +++ 4 files changed, 298 insertions(+), 12 deletions(-) Index: Makefile.in === RCS file: /cvs/src/src/winsup/utils/Makefile.in,v retrieving revision 1.69 diff -u -p -r1.69 Makefile.in --- Makefile.in 8 Mar 2008 17:52:49 - 1.69 +++ Makefile.in 9 Mar 2008 03:01:17 - @@ -99,10 +99,23 @@ else all: warn_cygcheck_zlib endif -# the rest of this file contains generic rules - all: Makefile $(CYGWIN_BINS) $(MINGW_BINS) +# test harness support (note: the MINGW_BINS += should come after the +# all: above so that the testsuite is not run for make but only +# make check.) +MINGW_BINS += testsuite.exe +MINGW_OBJS += path-testsuite.o testsuite.o +testsuite.exe: path-testsuite.o +path-testsuite.cc: path.cc ; @test -L $@ || ln -sf ${filter %.cc,$^} $@ +path-testsuite.o: MINGW_CXXFLAGS += -DTESTSUITE +# this is necessary because this .c lives in the build dir instead of src +path-testsuite.o: MINGW_CXX := ${patsubst -I.,-I$(utils_source),$(MINGW_CXX)} +path-testsuite.cc path.cc testsuite.cc: testsuite.h +check: testsuite.exe ; $(D)/$(F) + +# the rest of this file contains generic rules + # how to compile a MinGW object $(MINGW_OBJS): %.o: %.cc ifdef VERBOSE @@ -137,7 +150,7 @@ $(MINGW_BINS): $(MINGW_DEP_LDLIBS) $(CYGWIN_BINS): $(ALL_DEP_LDLIBS) clean: - rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) + rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) path-testsuite.cc testsuite.exe realclean: clean rm -f Makefile config.cache Index: path.cc === RCS file: /cvs/src/src/winsup/utils/path.cc,v retrieving revision 1.12 diff -u -p -r1.12 path.cc --- path.cc 4 Jun 2007 01:57:16 - 1.12 +++ path.cc 9 Mar 2008 03:01:17 - @@ -1,6 +1,6 @@ /* path.cc - Copyright 2001, 2002, 2003, 2005, 2006, 2007 Red Hat, Inc. + Copyright 2001, 2002, 2003, 2005, 2006, 2007, 2008 Red Hat, Inc. This file is part of Cygwin. @@ -22,6 +22,7 @@ details. */ #include cygwin/include/cygwin/version.h #include cygwin/include/sys/mount.h #include cygwin/include/mntent.h +#include testsuite.h /* Used when treating / and \ as equivalent. */ #define isslash(ch) \ @@ -227,16 +228,27 @@ readlink (HANDLE fh, char *path, int max return true; } -static struct mnt +typedef struct mnt { const char *native; char *posix; unsigned flags; int issys; - } mount_table[255]; + } mnt_t; + +#ifndef TESTSUITE +static mnt_t mount_table[255]; +#else +# define TESTSUITE_MOUNT_TABLE +# include testsuite.h +# undef
Re: [patch] utils/path.cc fixes and testsuite
On Sat, Mar 08, 2008 at 07:10:03PM -0800, Brian Dessent wrote: Christopher Faylor wrote: Would it be possible to uncollapse this if block and make it a little clearer? The else nine lines away makes it a little hard to follow. So, wouldn't just inverting the if (match) to if (!match) and putting the else condition cause some unnesting? Here's a v2 patch. Changes: - As suggested I used path[max_len] instead of *(path + max_len). I had done it that way originally to try to make it clear that I was looking at what the first chararacter of the path + max_len argument to concat, but I now agree it's kind of unidiomatic C. - Rearranged the if/else block, and added a comment for the logic of each branch. - Added a test for UNC paths. - Minor tweak on the Makefile rule that symlinks the source file to another name to prevent it from running every time. In general I'm not all that crazy with this idea of symlinking a file in order to compile it to a differently-named object, but doing it otherwise would require repeating the compile rule with all its ugly VERBOSE casing and I just went to the trouble to eliminate all such repetition in the Makefile. Brian 2008-03-08 Brian Dessent [EMAIL PROTECTED] * Makefile.in: Add a 'check' target that builds and runs testsuite.exe from path-testsuite.o and testsuite.o. * path.cc: Include testsuite.h. (struct mnt): Change to a mnt_t typedef and don't define mount_table when TESTSUITE is defined. (find2): Don't include when TESTSUITE is defined to avoid warning. (get_cygdrive0): Ditto. (get_cygdrive): Ditto. (read_mounts): Provide empty implementation when TESTSUITE is defined. (vconcat): Use the isslash macro. (unconvert_slashes): New helper to convert to backslashses. (rel_vconcat): Handle relative paths more gracefully. (cygpath): Skip a leading ./ sequence. Avoid double-slashes. Normalize final output to backslashes and remove redundant path sequences. * testsuite.cc: New file implementing testsuite driver. * testsuite.h: New header implementing harness mount table and series of tests. Makefile.in | 19 +++- path.cc | 71 +++ testsuite.cc | 89 testsuite.h | 131 +++ 4 files changed, 298 insertions(+), 12 deletions(-) Index: Makefile.in === RCS file: /cvs/src/src/winsup/utils/Makefile.in,v retrieving revision 1.69 diff -u -p -r1.69 Makefile.in --- Makefile.in8 Mar 2008 17:52:49 - 1.69 +++ Makefile.in9 Mar 2008 03:01:17 - @@ -99,10 +99,23 @@ else all: warn_cygcheck_zlib endif -# the rest of this file contains generic rules - all: Makefile $(CYGWIN_BINS) $(MINGW_BINS) +# test harness support (note: the MINGW_BINS += should come after the +# all: above so that the testsuite is not run for make but only +# make check.) +MINGW_BINS += testsuite.exe +MINGW_OBJS += path-testsuite.o testsuite.o +testsuite.exe: path-testsuite.o +path-testsuite.cc: path.cc ; @test -L $@ || ln -sf ${filter %.cc,$^} $@ +path-testsuite.o: MINGW_CXXFLAGS += -DTESTSUITE +# this is necessary because this .c lives in the build dir instead of src +path-testsuite.o: MINGW_CXX := ${patsubst -I.,-I$(utils_source),$(MINGW_CXX)} +path-testsuite.cc path.cc testsuite.cc: testsuite.h +check: testsuite.exe ; $(D)/$(F) + +# the rest of this file contains generic rules + # how to compile a MinGW object $(MINGW_OBJS): %.o: %.cc ifdef VERBOSE @@ -137,7 +150,7 @@ $(MINGW_BINS): $(MINGW_DEP_LDLIBS) $(CYGWIN_BINS): $(ALL_DEP_LDLIBS) clean: - rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) + rm -f *.o $(CYGWIN_BINS) $(MINGW_BINS) path-testsuite.cc testsuite.exe realclean: clean rm -f Makefile config.cache Index: path.cc === RCS file: /cvs/src/src/winsup/utils/path.cc,v retrieving revision 1.12 diff -u -p -r1.12 path.cc --- path.cc4 Jun 2007 01:57:16 - 1.12 +++ path.cc9 Mar 2008 03:01:17 - @@ -1,6 +1,6 @@ /* path.cc - Copyright 2001, 2002, 2003, 2005, 2006, 2007 Red Hat, Inc. + Copyright 2001, 2002, 2003, 2005, 2006, 2007, 2008 Red Hat, Inc. This file is part of Cygwin. @@ -22,6 +22,7 @@ details. */ #include cygwin/include/cygwin/version.h #include cygwin/include/sys/mount.h #include cygwin/include/mntent.h +#include testsuite.h /* Used when treating / and \ as equivalent. */ #define isslash(ch) \ @@ -227,16 +228,27 @@ readlink (HANDLE fh, char *path, int max return true; } -static struct mnt +typedef struct mnt { const char *native; char *posix; unsigned flags; int issys; - } mount_table[255]; + } mnt_t; + +#ifndef TESTSUITE +static mnt_t mount_table[255]; +#else +# define