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

2008-03-15 Thread Brian Dessent 
Corinna Vinschen wrote:

 Yuk.  I guess it would help a lot to reproduce path.cc:check_shortcut(*)
 in utils as close as possible.  Afaics it doesn't use any code which
 would be restricted to Cygwin, except for the call to
 mount_table-conv_to_posix_path in the posixify method.

I started down that road, trying to come up with a slick method for
sharing the code so that we don't need to maintain two copies.  After
poking at it for a while I came to the realization that the two are just
plumbed too differently to share code directly.  In the Cygwin code, the
check if this is a valid symlink and the read its contents are kind
of lumped together as the symlink_info class provides a convenient
location to store the data.  But in the cygcheck implementation we have
only is_symlink() and readlink(), with both take just a HANDLE and share
nothing, and no pflags to worry about, etc.

And of course there's the obvious that cygcheck has no Win32-POSIX
conversion capability which makes it hard to support reading reparse
points because readlink() is supposed to return the POSIX link target.

Anyway, the attached is the result of what happened when I started with
the Cygwin code and whittled it down.  It fixes the bug in the
grandparent of this email where it was reading the Win32 path out of a
shortcut that didn't have an ITEMIDLIST, and it supports the new-style
shortcut where the target  MAX_PATH gets stored at the end.  It does
not attempt to do anything with reparse points however.

Another factor was that the file IO in symlink_info::check_shortcut was
using the native API, which I rewrote to use the plain Win32 flavor in
case we want to keep cygcheck working on 9x/ME.  I don't think this will
matter if we want to make cygcheck support long paths though, since it's
just ReadFile, SetFilePointer, and GetFileInformationByHandle -- the
HANDLE is already open so this should require no change to support
WCHAR.

Brian2008-03-15  Brian Dessent  [EMAIL PROTECTED]

* path.cc: Include malloc.h for alloca.
(is_symlink): Rewrite.  Just read the whole file in memory rather
than by parts.  Account for an ITEMIDLIST if present, as well as
the new style of Cygwin shortcut supporting targets  MAX_PATH.

 path.cc |   96 +++-
 1 file changed, 47 insertions(+), 49 deletions(-)

Index: path.cc
===
RCS file: /cvs/src/src/winsup/utils/path.cc,v
retrieving revision 1.14
diff -u -p -r1.14 path.cc
--- path.cc 11 Mar 2008 17:20:02 -  1.14
+++ path.cc 15 Mar 2008 13:12:45 -
@@ -18,6 +18,7 @@ details. */
 #include windows.h
 #include stdio.h
 #include stdlib.h
+#include malloc.h
 #include path.h
 #include cygwin/include/cygwin/version.h
 #include cygwin/include/sys/mount.h
@@ -172,60 +173,57 @@ is_symlink (HANDLE fh)
 bool
 readlink (HANDLE fh, char *path, int maxlen)
 {
-  int got;
-  int magic = get_word (fh, 0x0);
+  DWORD rv;
+  char *buf, *cp;
+  unsigned short len;
+  win_shortcut_hdr *file_header;
+  BY_HANDLE_FILE_INFORMATION fi;
+
+  if (!GetFileInformationByHandle (fh, fi)
+  || fi.nFileSizeHigh != 0
+  || fi.nFileSizeLow  8192)
+return false;
 
-  if (magic == SHORTCUT_MAGIC)
-{
-  int offset = get_word (fh, 0x4c);
-  int slen = get_word (fh, 0x4c + offset + 2);
-  if (slen = maxlen)
-   {
- SetLastError (ERROR_FILENAME_EXCED_RANGE);
- return false;
-   }
-  if (SetFilePointer (fh, 0x4c + offset + 4, 0, FILE_BEGIN) ==
- INVALID_SET_FILE_POINTER  GetLastError () != NO_ERROR)
-   return false;
+  buf = (char *) alloca (fi.nFileSizeLow + 1);
+  file_header = (win_shortcut_hdr *) buf;
 
-  if (!ReadFile (fh, path, slen, (DWORD *) got, 0))
-   return false;
-  else if (got  slen)
-   {
- SetLastError (ERROR_READ_FAULT);
- return false;
-   }
-  else
-   path[got] = '\0';
-}
-  else if (magic == SYMLINK_MAGIC)
+  if (SetFilePointer (fh, 0L, NULL, FILE_BEGIN) == INVALID_SET_FILE_POINTER
+  || !ReadFile (fh, buf, fi.nFileSizeLow, rv, NULL)
+  || rv != fi.nFileSizeLow)
+return false;
+  
+  if (fi.nFileSizeLow  sizeof (file_header)
+   cmp_shortcut_header (file_header))
 {
-  char cookie_buf[sizeof (SYMLINK_COOKIE) - 1];
-
-  if (SetFilePointer (fh, 0, 0, FILE_BEGIN) == INVALID_SET_FILE_POINTER
-  GetLastError () != NO_ERROR)
-   return false;
-
-  if (!ReadFile (fh, cookie_buf, sizeof (cookie_buf), (DWORD *) got, 0))
-   return false;
-  else if (got == sizeof (cookie_buf)
-   memcmp (cookie_buf, SYMLINK_COOKIE, sizeof (cookie_buf)) == 0)
-   {
- if (!ReadFile (fh, path, maxlen, (DWORD *) got, 0))
-   return false;
- else if (got = maxlen)
-   {
- SetLastError (ERROR_FILENAME_EXCED_RANGE);
- path[0] = '\0';
- return

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

2008-03-11 Thread Corinna Vinschen 
On Mar 10 21:10, Corinna Vinschen wrote:
 On Mar 10 15:08, Christopher Faylor wrote:
  However, I don't understand what a mingw app would see when it is started
  from Cygwin now.  What would a standard windows app think that its cwd would
  be if it's cd'ed deep into a 32K long path.
 
 Right now, Cygwin copies its CWD into the user parameter block, as long
 as it is  260 chars.  When a Cygwin process cd's into a long path, this
 copy just doesn't happen.  So, the cwd of a MingW application started
 from that Cygwin process would be the last Cygwin cwd path  260
 characters within this process tree.
 
 This is what I started to discuss in
 http://cygwin.com/ml/cygwin-developers/2007-10/msg8.html
 
 As a result of this discussion we had five options what to do when
 spawning a native app from Cygwin, if the Cygwin process is in a long
 cwd:
 
 1. Return ENAMETOOLONG and don't start the native child.
 
 2. CWD for the native child is set to $SYSTEMROOT.
 
 3. CWD is set to the root of the current drive (either X:\ or
\\server\share).
 
 4. CWD is set to the longest leading path component of CWD which still
fits into MAX_PATH.
 
 5. Start the native app in the last CWD we were in which was  MAX_PATH.
 
 Implemented right now is option 5 since that's the most easy to implement
 because it practically didn't need any changes to the existing code.
 
 The general consensus seemed to lean towards option 1 or 3, so maybe
 this whole problem is going to be a non-issue...?
 
 Sigh, somehow it would be a pity if our own tools suffer from that
 restriction.

I applied a patch which enables option 1 for all long and virtual
paths.  So, for now, we don't have to take paths  MAX_PATH into
account for any MingW utility.


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-11 Thread Corinna Vinschen 
On Mar 11 08:36, Brian Dessent wrote:
 Corinna Vinschen wrote:
 
  Given that Cygwin changes to support long path names, I don't really
  like to see new code still using MAX_PATH and Win32 Ansi functions
  in the utils dir.  I know that the Win32 cwd is always restricted to
  259 chars.  However, there *is* a way to recognize the current working
  directory of the parent Cygwin application.
 
 Here's an updated version of the patch.  It addresses the problem of
 resolving symlink targets relative to the dir of the link without a
 gratuitous Get/SetCurrentDirectory sequence by introducing
 cygpath_rel().  It does not yet address the issue of modernizing the
 symlink reading code, that will follow.
 
 This patch still has two cases of MAX_PATH usage: one in a buffer that
 is used for GetCurrentDirectory(), so I don't see how making it larger
 there helps.  The second is in a static buffer for the dirname()
 helper.  I could simply make this one larger, however, the context where
 this dirname() is used is a filename that's used with CreateFile() so
 until that is plumbed to use WCHAR and \\?\ I don't really think it
 makes sense to use more than MAX_PATH.  I could update that instance of
 CreateFile, but the thing that provides the filename passed here is
 itself driven by GetFileAttributes(), so it would need to be updated
 too... and so on.  It looks like a lot of work to go completely
 longfile-clean here.

The patch looks basically ok with me.  Please check in.

Btw., you don't need to make the buffers MAX_PATH + 1.  MAX_PATH is
defined including the trailing NUL.  Existing code shows a lot of
irritation about this...


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-11 Thread Brian Dessent 
Corinna Vinschen wrote:

 Btw., you don't need to make the buffers MAX_PATH + 1.  MAX_PATH is
 defined including the trailing NUL.  Existing code shows a lot of
 irritation about this...

Oh, I wasn't even thinking of that... the reason I used MAX_PATH + 1 was
because earlier I had written

+  static char tmp[SYMLINK_MAX + 1];

so that the following sizes would not need to be SYMLINK_MAX - 1, 

+  if (!readlink (fh, tmp, SYMLINK_MAX))

+ strncpy (tmp, cygpath (papp, NULL), SYMLINK_MAX);

+ strncpy (lastsep+1, ptr, SYMLINK_MAX - (lastsep-tmp));


I.e. pure lazyness of wanting to type the least necessary.  But now that
you mention it, it makes more sense to have the - 1 than the + 1
form, so I'll change that.

Brian

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

2008-03-11 Thread Brian Dessent 
Corinna Vinschen wrote:

 Urgh.  MAX_PATH is defined with trailing 0, SYMLINK_MAX is defined
 without trailing 0 (like NAME_MAX).  You should better change the
 SYMLINK_MAX stuff back, afaics...

D'oh!  'Kay.

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

2008-03-10 Thread Corinna Vinschen 
On Mar 10 15:08, Christopher Faylor wrote:
 However, I don't understand what a mingw app would see when it is started
 from Cygwin now.  What would a standard windows app think that its cwd would
 be if it's cd'ed deep into a 32K long path.

Right now, Cygwin copies its CWD into the user parameter block, as long
as it is  260 chars.  When a Cygwin process cd's into a long path, this
copy just doesn't happen.  So, the cwd of a MingW application started
from that Cygwin process would be the last Cygwin cwd path  260
characters within this process tree.

This is what I started to discuss in
http://cygwin.com/ml/cygwin-developers/2007-10/msg8.html

As a result of this discussion we had five options what to do when
spawning a native app from Cygwin, if the Cygwin process is in a long
cwd:

1. Return ENAMETOOLONG and don't start the native child.

2. CWD for the native child is set to $SYSTEMROOT.

3. CWD is set to the root of the current drive (either X:\ or
   \\server\share).

4. CWD is set to the longest leading path component of CWD which still
   fits into MAX_PATH.

5. Start the native app in the last CWD we were in which was  MAX_PATH.

Implemented right now is option 5 since that's the most easy to implement
because it practically didn't need any changes to the existing code.

The general consensus seemed to lean towards option 1 or 3, so maybe
this whole problem is going to be a non-issue...?

Sigh, somehow it would be a pity if our own tools suffer from that
restriction.


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-09 Thread Corinna Vinschen 
Hi Brian,

Thanks for your patch.  I have a few nits, sorry.

On Mar  8 20:13, Brian Dessent wrote:
 Index: cygcheck.cc
 ===
 RCS file: /cvs/src/src/winsup/utils/cygcheck.cc,v
 retrieving revision 1.97
 diff -u -p -r1.97 cygcheck.cc
 --- cygcheck.cc   13 Jan 2008 13:41:45 -  1.97
 +++ cygcheck.cc   9 Mar 2008 03:52:07 -
 @@ -807,6 +807,31 @@ ls (char *f)
  display_error (ls: CloseHandle());
  }
  
 +/* If s is non-NULL, save the CWD in a static buffer and set the CWD
 +   to the dirname part of s.  If s is NULL, restore the CWD last
 +   saved.  */
 +static
 +void save_cwd_helper (const char *s)
 +{
 +  static char cwdbuf[MAX_PATH + 1];
 +  char dirnamebuf[MAX_PATH + 1];
 +
 +  if (s)
 +{
 +  GetCurrentDirectory (sizeof (cwdbuf), cwdbuf);
 +
 +  /* Remove the filename part from s.  */
 +  strncpy (dirnamebuf, s, MAX_PATH);
 +  dirnamebuf[MAX_PATH] = '\0';   // just in case strlen(s)  MAX_PATH
 +  char *lastsep = strrchr (dirnamebuf, '\\');
 +  if (lastsep)
 +lastsep[1] = '\0';
 +  SetCurrentDirectory (dirnamebuf);
 +}
 +  else
 +SetCurrentDirectory (cwdbuf);
 +}

Given that Cygwin changes to support long path names, I don't really
like to see new code still using MAX_PATH and Win32 Ansi functions
in the utils dir.  I know that the Win32 cwd is always restricted to
259 chars.  However, there *is* a way to recognize the current working
directory of the parent Cygwin application.

Bash as well as tcsh, as well as zsh (and probbaly pdksh, too) create an
environment variable $PWD.  Maybe Cygwin should create $PWD for native
apps if it's not already available through the parent shell.  I'd
suggest that the Cygwin utils first try to fetch $PWD from the
environment and use that as cwd.  Only if that fails, use
GetCurrentDirectory.

Never use SetCurrentDirectory, rather convert the path to an absolute
path, prepend \\?\ and call the Win32 unicode functions (CreateFileW,
etc).

  // Find a real application on the path (possibly following symlinks)
  static const char *
  find_app_on_path (const char *app, bool showall = false)
 @@ -822,25 +847,27 @@ find_app_on_path (const char *app, bool 
if (is_symlink (fh))
  {
static char tmp[4000] = ;

SYMLINK_MAX is PATH_MAX - 1 == 4095.

I'm wondering if you would like to tweak the readlink functions, too.
Cygwin shortcuts can now have the path name appended to the actual
shortcut data.  This hack was necessary to support pathnames longer than
2000 chars.  See the comment and code in cygwin/path.cc, line 3139ff.


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-09 Thread Brian Dessent 
Corinna Vinschen wrote:

 Given that Cygwin changes to support long path names, I don't really
 like to see new code still using MAX_PATH and Win32 Ansi functions
 in the utils dir.

Regardless of this patch, path.cc:rel_vconcat() currently uses
GetCurrentDirectory() to resolve relative paths.  It would be nice if
rel_vconcat() (or really, cygpath()) had an interface that let the
caller supply a CWD instead, as that would bypass the whole issue of
length since what this patch is doing is simply setting CWD just so that
rel_vconcat() can then get it again.  I thought about doing it that way
but it seemed more invasive.

 I know that the Win32 cwd is always restricted to
 259 chars.  However, there *is* a way to recognize the current working
 directory of the parent Cygwin application.
 
 Bash as well as tcsh, as well as zsh (and probbaly pdksh, too) create an
 environment variable $PWD.  Maybe Cygwin should create $PWD for native
 apps if it's not already available through the parent shell.  I'd
 suggest that the Cygwin utils first try to fetch $PWD from the
 environment and use that as cwd.  Only if that fails, use
 GetCurrentDirectory.

I will work on a patch that both adds an interface to allow the caller
to supply a CWD as well as trying to use $PWD to get the value
otherwise.

 Never use SetCurrentDirectory, rather convert the path to an absolute
 path, prepend \\?\ and call the Win32 unicode functions (CreateFileW,
 etc).

Setting the CWD can be totally avoided I think, by the above replumbing.

 SYMLINK_MAX is PATH_MAX - 1 == 4095.
 
 I'm wondering if you would like to tweak the readlink functions, too.
 Cygwin shortcuts can now have the path name appended to the actual
 shortcut data.  This hack was necessary to support pathnames longer than
 2000 chars.  See the comment and code in cygwin/path.cc, line 3139ff.

Oh, I didn't know that.  I'll add that to the list.

Brian

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

2008-03-09 Thread Brian Dessent 
Corinna Vinschen wrote:

   I'm wondering if you would like to tweak the readlink functions, too.
   Cygwin shortcuts can now have the path name appended to the actual
   shortcut data.  This hack was necessary to support pathnames longer than
   2000 chars.  See the comment and code in cygwin/path.cc, line 3139ff.
 
  Oh, I didn't know that.  I'll add that to the list.
 
 Thanks again.  I'm finally seeing light at the end of the long path
 name tunnel :)

Actually I'm a little confused now.  It seems like the code in
utils/path.cc:readlink() reads the Win32 path out of shortcut symlinks
but the POSIX path out of old-style symlinks -- not that it has any
choice since they don't contain the win32 path.  If that is the case
(and assuming I'm reading the new long filename symlink code correctly)
then it doesn't need any chaging since the [path too long] workaround
only applies to the POSIX link target stored in the 'description' field,
right?

But the semantics of sometimes you get an absolute Win32 path,
sometimes you get a relative POSIX path that readlink() seems to
provide baffles my mind.  More and more it seems that a lot of how these
non-Cygwin tools successfully process paths happens seemingly out of
luck.  I'll have to go and research the callers of readlink() but it
seems like it should be always returning the POSIX target, since that's
the only sane behavior in the face of old and new styles.

Brian

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

2008-03-09 Thread Corinna Vinschen 
On Mar  9 03:32, Brian Dessent wrote:
 Corinna Vinschen wrote:
 
I'm wondering if you would like to tweak the readlink functions, too.
Cygwin shortcuts can now have the path name appended to the actual
shortcut data.  This hack was necessary to support pathnames longer than
2000 chars.  See the comment and code in cygwin/path.cc, line 3139ff.
  
   Oh, I didn't know that.  I'll add that to the list.
  
  Thanks again.  I'm finally seeing light at the end of the long path
  name tunnel :)
 
 Actually I'm a little confused now.  It seems like the code in
 utils/path.cc:readlink() reads the Win32 path out of shortcut symlinks
 but the POSIX path out of old-style symlinks -- not that it has any
 choice since they don't contain the win32 path.  If that is the case
 (and assuming I'm reading the new long filename symlink code correctly)
 then it doesn't need any chaging since the [path too long] workaround
 only applies to the POSIX link target stored in the 'description' field,
 right?

Now that you mention it... did you see the comment in path.cc, line 3112ff?
There's a good chance that Windows shortcuts are not capable of long path
names.  I didn't test it so far, but it would be certainly better for
readlink to use the POSIX path in the symlink either way.


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-09 Thread Brian Dessent 
Corinna Vinschen wrote:

 Now that you mention it... did you see the comment in path.cc, line 3112ff?
 There's a good chance that Windows shortcuts are not capable of long path
 names.  I didn't test it so far, but it would be certainly better for
 readlink to use the POSIX path in the symlink either way.

Check this out.  I modified cygcheck to have a command line option to
dump out whatever readlink() returns.

$ cd /tmp
$ echo fileone fileone
$ ln -s fileone linkone
$ ln -s filetwo linktwo# filetwo doesn't exist yet
$ echo filetwo filetwo
$ cat linkone
fileone
$ cat linktwo
filetwo
$ ./cygcheck -x linkone linktwo
linkone - fileone
linktwo - c:\tmp\filetwo
$ ls -l linkone linktwo
lrwxrwxrwx 1 brian None 7 Mar  9 04:56 linkone - fileone
lrwxrwxrwx 1 brian None 7 Mar  9 04:56 linktwo - filetwo

So, the fact that the link was dangling at the time it was created
caused readlink to read it as a Win32 path -- which also causes it to
now be an absolute target instead of a relative target.  Apparently this
is due to the fact that if the target doesn't exist at creation time the
ParseDisplayName thing fails which results in a different structure for
the .lnk file, which confuses readlink()'s puny little brain which only
knows how to look at a fixed offset in the file, which results in it
reading a different slot.  Sigh.

Brian

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

2008-03-09 Thread Corinna Vinschen 
On Mar  9 05:02, Brian Dessent wrote:
 Corinna Vinschen wrote:
 
  Now that you mention it... did you see the comment in path.cc, line 3112ff?
  There's a good chance that Windows shortcuts are not capable of long path
  names.  I didn't test it so far, but it would be certainly better for
  readlink to use the POSIX path in the symlink either way.
 
 Check this out.  I modified cygcheck to have a command line option to
 dump out whatever readlink() returns.
 
 $ cd /tmp
 $ echo fileone fileone
 $ ln -s fileone linkone
 $ ln -s filetwo linktwo# filetwo doesn't exist yet
 $ echo filetwo filetwo
 $ cat linkone
 fileone
 $ cat linktwo
 filetwo
 $ ./cygcheck -x linkone linktwo
 linkone - fileone
 linktwo - c:\tmp\filetwo
 $ ls -l linkone linktwo
 lrwxrwxrwx 1 brian None 7 Mar  9 04:56 linkone - fileone
 lrwxrwxrwx 1 brian None 7 Mar  9 04:56 linktwo - filetwo
 
 So, the fact that the link was dangling at the time it was created
 caused readlink to read it as a Win32 path -- which also causes it to
 now be an absolute target instead of a relative target.  Apparently this
 is due to the fact that if the target doesn't exist at creation time the
 ParseDisplayName thing fails which results in a different structure for
 the .lnk file, which confuses readlink()'s puny little brain which only
 knows how to look at a fixed offset in the file, which results in it
 reading a different slot.  Sigh.

Yuk.  I guess it would help a lot to reproduce path.cc:check_shortcut(*)
in utils as close as possible.  Afaics it doesn't use any code which
would be restricted to Cygwin, except for the call to
mount_table-conv_to_posix_path in the posixify method.


Corinna

(*) and, FWIW, symlink_info::check_reparse_point and
symlink_info::check_sysfile.

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-09 Thread Christopher Faylor 
On Sun, Mar 09, 2008 at 10:28:06AM +0100, Corinna Vinschen wrote:
Hi Brian,

Thanks for your patch.  I have a few nits, sorry.

On Mar  8 20:13, Brian Dessent wrote:
 Index: cygcheck.cc
 ===
 RCS file: /cvs/src/src/winsup/utils/cygcheck.cc,v
 retrieving revision 1.97
 diff -u -p -r1.97 cygcheck.cc
 --- cygcheck.cc  13 Jan 2008 13:41:45 -  1.97
 +++ cygcheck.cc  9 Mar 2008 03:52:07 -
 @@ -807,6 +807,31 @@ ls (char *f)
  display_error (ls: CloseHandle());
  }
  
 +/* If s is non-NULL, save the CWD in a static buffer and set the CWD
 +   to the dirname part of s.  If s is NULL, restore the CWD last
 +   saved.  */
 +static
 +void save_cwd_helper (const char *s)
 +{
 +  static char cwdbuf[MAX_PATH + 1];
 +  char dirnamebuf[MAX_PATH + 1];
 +
 +  if (s)
 +{
 +  GetCurrentDirectory (sizeof (cwdbuf), cwdbuf);
 +
 +  /* Remove the filename part from s.  */
 +  strncpy (dirnamebuf, s, MAX_PATH);
 +  dirnamebuf[MAX_PATH] = '\0';   // just in case strlen(s)  MAX_PATH
 +  char *lastsep = strrchr (dirnamebuf, '\\');
 +  if (lastsep)
 +lastsep[1] = '\0';
 +  SetCurrentDirectory (dirnamebuf);
 +}
 +  else
 +SetCurrentDirectory (cwdbuf);
 +}

Given that Cygwin changes to support long path names, I don't really
like to see new code still using MAX_PATH and Win32 Ansi functions
in the utils dir.  I know that the Win32 cwd is always restricted to
259 chars.  However, there *is* a way to recognize the current working
directory of the parent Cygwin application.

Bash as well as tcsh, as well as zsh (and probbaly pdksh, too) create an
environment variable $PWD.  Maybe Cygwin should create $PWD for native
apps if it's not already available through the parent shell.

I'd really rather not do that.  I don't think Cygwin should be polluting
the environment any more than it has to.  Even if this worked, it is
easily defeatable by setting a PWD environment variable before running
cygwin, so you'd have to keep track of this value through multiple
levels of process invocation.

I know everyone hates it but a cygwin_internal interface could be used
to get the current working directory for applications that need it.  I
would think that only mingw-like applications working in close
conjunction with cygwin would care about this.

cgf

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

2008-03-09 Thread Corinna Vinschen 
On Mar  9 10:38, Christopher Faylor wrote:
 On Sun, Mar 09, 2008 at 10:28:06AM +0100, Corinna Vinschen wrote:
 Bash as well as tcsh, as well as zsh (and probbaly pdksh, too) create an
 environment variable $PWD.  Maybe Cygwin should create $PWD for native
 apps if it's not already available through the parent shell.
 
 I'd really rather not do that.  I don't think Cygwin should be polluting
 the environment any more than it has to.  Even if this worked, it is
 easily defeatable by setting a PWD environment variable before running
 cygwin, so you'd have to keep track of this value through multiple
 levels of process invocation.

Nothing against adding a cygwin_internal for such a purpose, but how is
that going to work?  Assuming the MingW application is called from a
Cygwin application.  If the parent application's cwd is a long path, the
MingW child gets its own cwd for apparent reasons.  If it loads the
cygwin DLL dynamically, as cygcheck already does, how is that new
invocation of the DLL supposed to know the cwd of the parent process? 

Maybe I miss something, but I don't see how that could work without
using another mechanism to transmit the cwd of the parent application to
the child, and the environment seems like a pretty simple way to do it.

What about just using PWD if it's set, but not to invent it in Cygwin if
it doesn't exist?  I see two situations which probably cover 99% of the
cases. 

The first case is starting cygcheck from the native command shell.  That
shell doesn't understand long paths anyway and always has a cwd which
can be fetched by GetCurrentDirectory.

The second case is starting cygcheck from a Cygwin shell.  The Cygwin
shell sets $PWD anyway, at least bash, tcsh and zsh.


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-09 Thread Christopher Faylor 
On Sun, Mar 09, 2008 at 04:14:40PM +0100, Corinna Vinschen wrote:
On Mar  9 10:38, Christopher Faylor wrote:
 On Sun, Mar 09, 2008 at 10:28:06AM +0100, Corinna Vinschen wrote:
 Bash as well as tcsh, as well as zsh (and probbaly pdksh, too) create an
 environment variable $PWD.  Maybe Cygwin should create $PWD for native
 apps if it's not already available through the parent shell.
 
 I'd really rather not do that.  I don't think Cygwin should be polluting
 the environment any more than it has to.  Even if this worked, it is
 easily defeatable by setting a PWD environment variable before running
 cygwin, so you'd have to keep track of this value through multiple
 levels of process invocation.

Nothing against adding a cygwin_internal for such a purpose, but how is
that going to work?  Assuming the MingW application is called from a
Cygwin application.  If the parent application's cwd is a long path, the
MingW child gets its own cwd for apparent reasons.  If it loads the
cygwin DLL dynamically, as cygcheck already does, how is that new
invocation of the DLL supposed to know the cwd of the parent process? 

I guess I misunderstood.  I thought that the current working directory
could be derived through some complicated combination of Nt*() calls.

Maybe I miss something, but I don't see how that could work without
using another mechanism to transmit the cwd of the parent application to
the child, and the environment seems like a pretty simple way to do it.

What about just using PWD if it's set, but not to invent it in Cygwin if
it doesn't exist?  I see two situations which probably cover 99% of the
cases. 

The problem with environment variables are that they are under the
user's control.  If the user decides to modify PWD, then it can cause
issues.  That's why I was looking for a more foolproof solution.

That + I hate the thought of adding YA special environment variable for
Cygwin to worry about.  A real OS would never consider passing stuff
around in environment variables.

cgf

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

2008-03-09 Thread Brian Dessent 
Christopher Faylor wrote:

 I guess I misunderstood.  I thought that the current working directory
 could be derived through some complicated combination of Nt*() calls.

I could be wrong here but the way I understood it, there is no concept
of a working directory at the NT level, that is something that is
maintained by the Win32 layer.

My question is, what does GetCurrentDirectoryW() return if the current
directory is greater than the 260 limit?  Does it choke or does it
switch to the \.\c:\foo syntax?

Brian

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

2008-03-09 Thread Corinna Vinschen 
On Mar  9 11:03, Brian Dessent wrote:
 Christopher Faylor wrote:
 
  I guess I misunderstood.  I thought that the current working directory
  could be derived through some complicated combination of Nt*() calls.
 
 I could be wrong here but the way I understood it, there is no concept
 of a working directory at the NT level, that is something that is
 maintained by the Win32 layer.

That's right.  NT doesn't have a notion what a cwd is.  It only has the
OBJECT_ATTRIBUTES structure which defines an object by an absolute path,
or by a path relative to a directory handle.

The cwd is maintained by kernel32.dll in a per-process structure called
RTL_USER_PROCESS_PARAMETERS.  The cwd is stored as path (always with
trailing backslash) and as handle.

Sometimes the Win32 functions use the cwd's path to create an absolute
path from a relative path, sometimes they use the cwd's handle and the
relative path in calls to NT functions.

 My question is, what does GetCurrentDirectoryW() return if the current
 directory is greater than the 260 limit?  Does it choke or does it
 switch to the \.\c:\foo syntax?

It can't do that.  See the MSDN man page for SetCurrentDirectory:
http://msdn2.microsoft.com/en-us/library/aa365530(VS.85).aspx

 The string must not exceed MAX_PATH characters including the
  terminating null character.

The problem is that the cwd is stored as UNICODE_STRING with a
statically allocated buffer in the user parameter block.  The
MaximumLength is set to 520.  SetCurrentDirectory refuses to take paths
longer than that.  In theory it would be possible to define a longer cwd
by defining a new buffer in the cwd's UNICODE_STRING.  But I never tried
that.  I don't really know if that's possible and what happens if you
call CreateProcess or, FWIW, any Win32 file access function after doing
that.  Nobody keeps us from trying, but I have this gut feeling that
different NT versions will show different behaviour...


Corinna


(*) See cygwin/ntdll.h, struct _RTL_USER_PROCESS_PARAMETERS.

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

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

2008-03-09 Thread Christopher Faylor 
On Sun, Mar 09, 2008 at 08:55:09PM +0100, Corinna Vinschen wrote:
On Mar  9 11:03, Brian Dessent wrote:
 Christopher Faylor wrote:
 
  I guess I misunderstood.  I thought that the current working directory
  could be derived through some complicated combination of Nt*() calls.
 
 I could be wrong here but the way I understood it, there is no concept
 of a working directory at the NT level, that is something that is
 maintained by the Win32 layer.

That's right.  NT doesn't have a notion what a cwd is.  It only has the
OBJECT_ATTRIBUTES structure which defines an object by an absolute path,
or by a path relative to a directory handle.

The cwd is maintained by kernel32.dll in a per-process structure called
RTL_USER_PROCESS_PARAMETERS.  The cwd is stored as path (always with
trailing backslash) and as handle.

Duh, right.  I knew that.  I've seen the code.

So, maybe we could make sure the handle was inherited and pass it along
in a _CYGWIN_PWD=0x239487 format to the child?

cgf

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

2008-03-09 Thread Brian Dessent 
Corinna Vinschen wrote:

 The problem is that the cwd is stored as UNICODE_STRING with a
 statically allocated buffer in the user parameter block.  The
 MaximumLength is set to 520.  SetCurrentDirectory refuses to take paths
 longer than that.  In theory it would be possible to define a longer cwd
 by defining a new buffer in the cwd's UNICODE_STRING.  But I never tried
 that.  I don't really know if that's possible and what happens if you
 call CreateProcess or, FWIW, any Win32 file access function after doing
 that.  Nobody keeps us from trying, but I have this gut feeling that
 different NT versions will show different behaviour...

How does this fit in with the practice of maintaining our own Cygwin CWD
state separate from the Win32 CWD so that unlink(.) and the like can
succeed?  Or is that precisely how we are able to support a CWD = 260? 
If it is the case that we can only support a CWD = 260 by faking it,
i.e. not trying to sync the Win32 CWD to the actual long CWD, then it
seems we are limited to two choices for how to deal with a long CWD in a
non-Cygwin process: a) not supported, b) supported only through some
special hook (such as cgf's idea of handle inheritance of the Cygwin CWD
handle) or through relying on the shell to set PWD.

Brian

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

2008-03-08 Thread Brian Dessent 
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.  

Brian2008-03-08  Brian Dessent  [EMAIL PROTECTED]

* cygcheck.cc (save_cwd_helper): New helper function for saving
the current directory.
(find_app_on_path): Use sizeof instead of hardcoded array sizes
throughout.  Change into the dir of the link when normalizing
its target.  Don't worry about converting slashes as cygpath ()
now handles that.

 cygcheck.cc |   45 -
 1 file changed, 36 insertions(+), 9 deletions(-)

Index: cygcheck.cc
===
RCS file: /cvs/src/src/winsup/utils/cygcheck.cc,v
retrieving revision 1.97
diff -u -p -r1.97 cygcheck.cc
--- cygcheck.cc 13 Jan 2008 13:41:45 -  1.97
+++ cygcheck.cc 9 Mar 2008 03:52:07 -
@@ -807,6 +807,31 @@ ls (char *f)
 display_error (ls: CloseHandle());
 }
 
+/* If s is non-NULL, save the CWD in a static buffer and set the CWD
+   to the dirname part of s.  If s is NULL, restore the CWD last
+   saved.  */
+static
+void save_cwd_helper (const char *s)
+{
+  static char cwdbuf[MAX_PATH + 1];
+  char dirnamebuf[MAX_PATH + 1];
+
+  if (s)
+{
+  GetCurrentDirectory (sizeof (cwdbuf), cwdbuf);
+
+  /* Remove the filename part from s.  */
+  strncpy (dirnamebuf, s, MAX_PATH);
+  dirnamebuf[MAX_PATH] = '\0';   // just in case strlen(s)  MAX_PATH
+  char *lastsep = strrchr (dirnamebuf, '\\');
+  if (lastsep)
+lastsep[1] = '\0';
+  SetCurrentDirectory (dirnamebuf);
+}
+  else
+SetCurrentDirectory (cwdbuf);
+}
+
 // Find a real application on the path (possibly following symlinks)
 static const char *
 find_app_on_path (const char *app, bool showall = false)
@@ -822,25 +847,27 @@ find_app_on_path (const char *app, bool 
   if (is_symlink (fh))
 {
   static char tmp[4000] = ;
-  char *ptr;
-  if (!readlink (fh, tmp, 3999))
+  if (!readlink (fh, tmp, sizeof (tmp)-1))
display_error(readlink failed);
-  ptr = cygpath (tmp, NULL);
-  for (char *p = ptr; (p = strchr (p, '/')); p++)
-   *p = '\\';
+  
+  /* When resolving the linkname, set the CWD to the context of
+ the link, so that relative links are correctly resolved.  */
+  save_cwd_helper (papp);
+  char *ptr = cygpath (tmp, NULL);
+  save_cwd_helper (NULL);
   printf ( - %s\n, ptr);
   if (!strchr (ptr, '\\'))
{
  char *lastsep;
- strncpy (tmp, cygpath (papp, NULL), 3999);
- for (char *p = tmp; (p = strchr (p, '/')); p++)
-   *p = '\\';
+ strncpy (tmp, cygpath (papp, NULL), sizeof (tmp)-1);
  lastsep = strrchr (tmp, '\\');
- strncpy (lastsep+1, ptr, 3999-(lastsep-tmp));
+ strncpy (lastsep+1, ptr, (sizeof (tmp)-1) - (lastsep-tmp));
  ptr = tmp;
}
   if (!CloseHandle (fh))
display_error (find_app_on_path: CloseHandle());
+  /* FIXME: We leak the ptr returned by cygpath() here which is a
+ malloc()d string.  */
   return find_app_on_path (ptr, showall);
 }