[patch] handle_to_fn: null terminate

I noticed in strace some lines like:

fhandler_base::close: closing
'/Device/NamedPipe/Win32Pipes.000008e0.00000002<several junk bytes>'
handle 0x740

This was caused by handle_to_fn simply forgetting to add a \0 when
converting, as in the attached patch.

Brian

2008-03-07  Brian Dessent  <brian <at> dessent.net>

	* dtable.cc (handle_to_fn): Null-terminate posix_fn in the case
	of justslash = true.

Index: dtable.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/dtable.cc,v
retrieving revision 1.182
diff -u -p -r1.182 dtable.cc
--- dtable.cc	15 Feb 2008 17:53:10 -0000	1.182
+++ dtable.cc	8 Mar 2008 01:33:52 -0000
 <at>  <at>  -952,6 +952,7  <at>  <at>  handle_to_fn (HANDLE h, char *posix_fn)
 	  *d = '/';
 	else
 	  *d = *s;
+      *d = 0;
     }

Re: [patch] handle_to_fn: null terminate

On Fri, Mar 07, 2008 at 05:38:58PM -0800, Brian Dessent wrote:
>
>I noticed in strace some lines like:
>
>fhandler_base::close: closing
>'/Device/NamedPipe/Win32Pipes.000008e0.00000002<several junk bytes>'
>handle 0x740
>
>This was caused by handle_to_fn simply forgetting to add a \0 when
>converting, as in the attached patch.
>
>Brian
>2008-03-07  Brian Dessent  <brian <at> dessent.net>
>
>	* dtable.cc (handle_to_fn): Null-terminate posix_fn in the case
>	of justslash = true.
>
>Index: dtable.cc
>===================================================================
>RCS file: /cvs/src/src/winsup/cygwin/dtable.cc,v
>retrieving revision 1.182
>diff -u -p -r1.182 dtable.cc
>--- dtable.cc	15 Feb 2008 17:53:10 -0000	1.182
>+++ dtable.cc	8 Mar 2008 01:33:52 -0000
> <at>  <at>  -952,6 +952,7  <at>  <at>  handle_to_fn (HANDLE h, char *posix_fn)
> 	  *d = '/';
> 	else
> 	  *d = *s;
>+      *d = 0;
>     }

[patch] reorganize utils/Makefile.in

This patch is a revamping of the Makefile in winsup/utils.  The current
Makefile.in is fugly, in my humble opinion.  It's got lots of repeated
rules and it's not very clear how one is supposed to add or change
things.  This patch does use GNU make specific features, but I'm quite
sure we already use them in other places, e.g. $(wildcard ...),
$(patsubst ...), $(shell ...) etc. are all GNU make features AFAIK and
those are all over the place in winsup.

I've also attached a copy of the patched Makefile.in if it's easier to
review that way; the diff is quite ugly.  As you can see the total
number of lines in the file is decreased by about 70, and that's
including a number of comments that I have added to document how to use
the file.

I have tried fairly hard to make sure that no actual behavior has
changed.  I diffed a before and after run and the only real difference
seemed to be the order that things ran, as well as a couple of flags
moved to a different spot in their commands.  I also tested the case
where libbfd.a/libintl.a are absent, causing the dumper parts to be
skipped as well as missing a MinGW zlib for cygcheck.  I haven't tested
a crosscompile but I can if that's necessary.  I have tested builds in
both a combined tree as well as just winsup.

Brian

2008-03-08  Brian Dessent  <brian <at> dessent.net>

	* Makefile.in: Reorganize considerably, using GNU make's
	static pattern rules and target-specific variables.

Re: [patch] reorganize utils/Makefile.in

On Sat, Mar 08, 2008 at 07:28:52AM -0800, Brian Dessent wrote:
>2008-03-08  Brian Dessent  <brian <at> dessent.net>
>
>	* Makefile.in: Reorganize considerably, using GNU make's
>	static pattern rules and target-specific variables.

Looks good.  Please check in.

Thanks.

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

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

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.

Brian

2008-03-08  Brian Dessent  <brian <at> dessent.net>

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  <brian <at> dessent.net>
>

Re: [patch] cygcheck.cc update for cygpath()

Christopher Faylor wrote:

> It looks like yo can still unindent this by  changing the == to !=, putting
> the temppath under that and keeping all of the if's at the same level:

Oh, I see now what you mean.

> If the if block is that small, then I think I'd prefer just one comment
> at the beginning which describes what it is doing.  Otherwise, I got
> lost in what was happening while trying to see where the comments line
> up.  I don't feel really strongly about that, though, so feel free to
> ignore me.  I would prefer not having the nested if's though.
> 
> Otherwise, this looks good.  If you make the above suggestions, feel
> free to check this in.

I chopped each comment down to a single line //-style which I think
helps the clutter, and removed the nesting.

Also, there are a few tweaks to cygcheck.cc necessary as a result, see
attached.  The main idea is that when normalizing a link's target in
find_app_on_path, we should temporarily set the CWD to the same dir as
the link, otherwise relative links will get normalized relative to
whatever dir cygcheck was run from.  

Brian

2008-03-08  Brian Dessent  <brian <at> dessent.net>

	* cygcheck.cc (save_cwd_helper): New helper function for saving

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 -0000	1.69
> >+++ Makefile.in	9 Mar 2008 03:01:17 -0000
> > <at>  <at>  -99,10 +99,23  <at>  <at>  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