Re: thunking, the next step
On Sat, 2003-11-22 at 11:26, Warren Young wrote: Robert Collins wrote: http://www.jargon.8hz.com/jargon_35.html - see thunk, and meaning 3 is what I've been using. jargon.org is the official site, and it has a more detailed definition: http://www.catb.org/~esr/jargon/html/T/thunk.html Uhm, thats catb.org :}. But yes, I did dig up an older copy of the TNHD. Beware of definition 4, which can cause confusion in the Windows environment. It's the one I thought was being referred to when I first saw this subject line. Identical in concept though: you call a 16 bit function, and a 32 bit function is executed - via the thunk. Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: thunking, the next step
On Fri, 2003-11-21 at 22:02, Corinna Vinschen wrote: Ok. Well for now, I'm going to leave the thunks in place, until / if they become nothing more than if (unicode) ...W() else A(). That said, all the calls we are thunking require kernel mode transitions, so I really don't believe that the thunking will add any overhead on it's own: the context switch going into kernel will obliterate the much smaller overhead of checking which call we want to make. I don't think so. You can't take the kernel into account, really, since it spends its time either case. Well, thats irrelevant anyway. I've already committed to moving the duplication of tests out of the call path. Anyway, *sic* I don't like the thunking. It's fairly intrusive to the code. It adds another complexity level to a lot of functions which seems pretty unnecessary. It also adds a lot of decisions which are made on runtime over and over again, even though actually it would be sufficient from a logical level to make this decision once. Or at least only once per Win32 function call. Those decisions can live elsewhere - see above. For now, it provides me a clean way to tell what has been fixed, and what not - and to insert tracing and debugging statements at *one* place in the code (per win32 function) rather than at multiple places. Btw., what does thunk mean literally? While I know its meaning in the software context, I can't find a simple translation. I looked up three dictionaries to no avail. http://www.jargon.8hz.com/jargon_35.html - see thunk, and meaning 3 is what I've been using. Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: For masochists: the leap o faith
On Sat, 2003-11-15 at 09:07, Christopher Faylor wrote: This problem is fixed in the gcc cvs trunk. I've asked Danny and Gerrit about backporting the fix to 3.3.2. It should be trivial to do so. Any word on this? Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: thunking, the next step
On Mon, 2003-11-17 at 23:02, Corinna Vinschen wrote: And, structures like the FindNext* details change in definition when UNICODE is defined. I was trying to avoid all that complexity, which is significant, by staying in a thunk approach. Yep, I agree, that's an extra problem. But it doesn't invalidate the general idea of putting the work into autoload and path_conv. The FindFile example might be something which justifies the use of wrapper functions for these very cases. Ok. Well for now, I'm going to leave the thunks in place, until / if they become nothing more than if (unicode) ...W() else A(). That said, all the calls we are thunking require kernel mode transitions, so I really don't believe that the thunking will add any overhead on it's own: the context switch going into kernel will obliterate the much smaller overhead of checking which call we want to make. I decided against redefining the 'real' calls because I figured some areas may want to use the 'real' calls directly, and only the auto-adjusting parts of cygwin should have the ansi/wide dichotomy. I don't know if I understand you right. I was only talking about calls which are affecting the file system. Other calls like CreateSemaphore or what not should still work as before. The autoload part would define some new LoadDLLfuncBLURB which is used only for the affected functions. I (and I assume cgf) was not talking about using that approach for all functions with an ascii and a wide char implementation. Never mind, thinko on my part: the A and W versions would still be directly accesible. Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
ATTN Gerrit (Was Re: For masochists: the leap o faith)
On Fri, 2003-11-21 at 08:05, Christopher Faylor wrote: I backported it into cygwin's 3.3.2 branch but I haven't seen Gerrit around for a while to generate a new gcc. Cool, thanks. Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: ATTN Gerrit (Was Re: For masochists: the leap o faith)
On Fri, 2003-11-21 at 10:35, Gerrit P. Haase wrote: Hello Robert, On 20. November 2003 at 22:09 you wrote: RC On Fri, 2003-11-21 at 08:05, Christopher Faylor wrote: I backported it into cygwin's 3.3.2 branch but I haven't seen Gerrit around for a while to generate a new gcc. I'm out of office and it is a lot of work to do here in my company. I'll try to get 3.3.2 ready over the weekend. Cool - thank you. If I can help, let me know. Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: For masochists: the leap o faith
On Sat, 2003-11-15 at 15:43, Christopher Faylor wrote: Yes, I've already (obviously?) been to SUSv3. I wasn't talking about standards. I was talking about common practice. If you have a common practice web site that you want to show me then that might be a convincing argument. Otherwise, I'll have to fall back on my personal UNIX experience. http://zebra.fh-weingarten.de/~maxi/html/mplayer-dev-eng/2003-04/msg00600.html Part of a thread on this in another project. Seems like the hurd follows the no-PATH_MAX, use pathconf() always approach. Which means that everything thats portable to the hurd, will Do The Right Thing, if we eliminate the PATH_MAX and MAXPATHLEN defines. In my digging, I found that PATH_MAX, if defined, MUST be the largest path length possible. Presumably thats so that programs with static buffers won't run into trouble. Either way(increase PATH_MAX or remove it), to support longer paths, we'll need to make the change and transition to it. So, as I see it the question for you is : which route do you prefer? Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: The increased path length changes
On Sun, 2003-11-16 at 10:11, Christopher Faylor wrote: I'm not going to nickel and dime over what is mechanical and what isn't. We need an assignment from Ron. Ok. Ron? -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
thunk createDirectory and createFile calls
Ok, this is the thin edge of the wedge:
thunk CreateDirectory and CreateFile throughout the core.
All inline, so there should be no performance hit, but the coming
unicode changes will be able to be done in one place.
I considered redefining the CreateDirectory and CreateFile calls but
decided against it.
I'd like to commit these parts in, as they are
a) common to every approach to this problem I can think of.
b) safe.
Is that ok?
2003-11-11 Robert Collins [EMAIL PROTECTED]
Ron Parker [EMAIL PROTECTED]
Rename CreateFile to cygwin_create_file throughout.
Rename CreateDirectory to cygwin_create_directory throughout.
* assert.cc: Ditto.
* dcrt0.cc: Ditto.
* dir.cc: Ditto.
* exceptions.cc: Ditto.
* fhandler.cc: Ditto.
* fhandler_console.cc: Ditto.
* fhandler_disk_file.cc: Ditto.
* fhandler_proc.cc: Ditto.
* fhandler_socket.cc: Ditto.
* fork.cc: Ditto.
* ntea.cc: Ditto.
* path.cc: Ditto.
* security.cc: Ditto.
* spawn.cc: Ditto.
* syscalls.cc: Ditto.
* times.cc: Ditto.
* uinfo.cc: Ditto.
2003-11-11 Robert Collins [EMAIL PROTECTED]
Ron Parker [EMAIL PROTECTED]
Rename CreateFile to cygwin_create_file throughout.
Rename CreateDirectory to cygwin_create_directory throughout.
* assert.cc: Ditto.
* dcrt0.cc: Ditto.
* dir.cc: Ditto.
* exceptions.cc: Ditto.
* fhandler.cc: Ditto.
* fhandler_console.cc: Ditto.
* fhandler_disk_file.cc: Ditto.
* fhandler_proc.cc: Ditto.
* fhandler_socket.cc: Ditto.
* fork.cc: Ditto.
* ntea.cc: Ditto.
* path.cc: Ditto.
* security.cc: Ditto.
* spawn.cc: Ditto.
* syscalls.cc: Ditto.
* times.cc: Ditto.
* uinfo.cc: Ditto.
? cvs.exe.stackdump
? cygwin_daemon.patch
? io.h
? localdiff
? pthread_cancel.patch
? pthread_fix.patch
? pthread_fork_save_keys.patch
? pthread_mutex.patch
? t
Index: assert.cc
===
RCS file: /cvs/src/src/winsup/cygwin/assert.cc,v
retrieving revision 1.9
diff -u -p -r1.9 assert.cc
--- assert.cc 19 Sep 2002 15:12:48 - 1.9
+++ assert.cc 14 Nov 2003 09:36:36 -
@@ -12,6 +12,7 @@ details. */
#include security.h
#include wingdi.h
#include winuser.h
+#include io.h
#include assert.h
#include stdlib.h
@@ -28,8 +29,9 @@ __assert (const char *file, int line, co
/* If we don't have a console in a Windows program, then bring up a
message box for the assertion failure. */
- h = CreateFile (CONOUT$, GENERIC_WRITE, FILE_SHARE_WRITE, sec_none_nih,
- OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+ h = cygwin_create_file (CONOUT$, GENERIC_WRITE, FILE_SHARE_WRITE,
+ sec_none_nih, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL,
+ NULL);
if (h == INVALID_HANDLE_VALUE)
{
char *buf;
Index: dcrt0.cc
===
RCS file: /cvs/src/src/winsup/cygwin/dcrt0.cc,v
retrieving revision 1.189
diff -u -p -r1.189 dcrt0.cc
--- dcrt0.cc16 Oct 2003 14:08:27 - 1.189
+++ dcrt0.cc14 Nov 2003 09:36:36 -
@@ -34,6 +34,7 @@ details. */
#include dll_init.h
#include cygthread.h
#include sync.h
+#include io.h
#define MAX_AT_FILE_LEVEL 10
@@ -164,13 +165,13 @@ insert_file (char *name, char *cmd)
HANDLE f;
DWORD size;
- f = CreateFile (name + 1,
- GENERIC_READ, /* open for reading*/
- FILE_SHARE_READ, /* share for reading */
- sec_none_nih, /* no security */
- OPEN_EXISTING, /* existing file only */
- FILE_ATTRIBUTE_NORMAL, /* normal file */
- NULL); /* no attr. template */
+ f = cygwin_create_file (name + 1,
+ GENERIC_READ, /* open for reading*/
+ FILE_SHARE_READ, /* share for reading */
+ sec_none_nih, /* no security */
+ OPEN_EXISTING, /* existing file only */
+ FILE_ATTRIBUTE_NORMAL, /* normal file */
+ NULL); /* no attr. template */
if (f == INVALID_HANDLE_VALUE)
{
@@ -1107,9 +1108,9 @@ __api_fatal (const char *fmt, ...)
a serious error. */
if (GetFileType (GetStdHandle (STD_ERROR_HANDLE)) != FILE_TYPE_CHAR)
{
- HANDLE h = CreateFile (CONOUT$, GENERIC_READ | GENERIC_WRITE,
-FILE_SHARE_WRITE | FILE_SHARE_WRITE,
-sec_none, OPEN_EXISTING, 0, 0);
+ HANDLE h = cygwin_create_file (CONOUT$, GENERIC_READ | GENERIC_WRITE
Re: thunk createDirectory and createFile calls
Max Bowsher wrote: Also, I think LPCTSTR should be LPCSTR ? (and also in the CreateFile case) Not according to MSDN. (excuse the unsigned emails, I don't have gpg setup with thunderbird on cygwin :[) Rob
thunking, the next step
Ok, I've now integrated and generalised Ron's unicode support mini-patch.
So, here tis a version that, well the changelog explains the overview,
and io.h the detail.
Overhead wise, this is reasonably low:
1 strlen() per IO call minimum.
1 unicode conversion, only if needed.
inlined code, so no additional function calls.
2003-11-11 Robert Collins [EMAIL PROTECTED]
Ron Parker [EMAIL PROTECTED]
Rename thunked functions to cygwin_function_name,
create unicode capable thunks, and add autoload support, for:
CreateFile
CreateDirectory
SetFileAttributes
GetFileAttributes
MoveFile
MoveFileEx
* assert.cc: Ditto.
* dcrt0.cc: Ditto.
* dir.cc: Ditto.
* exceptions.cc: Ditto.
* fhandler.cc: Ditto.
* fhandler_console.cc: Ditto.
* fhandler_disk_file.cc: Ditto.
* fhandler_proc.cc: Ditto.
* fhandler_socket.cc: Ditto.
* fork.cc: Ditto.
* ntea.cc: Ditto.
* path.cc: Ditto.
* security.cc: Ditto.
* spawn.cc: Ditto.
* syscalls.cc: Ditto.
* times.cc: Ditto.
* uinfo.cc: Ditto.
* autoload.cc: Add ...W functions for the thunked functions.
* wincap.cc: Add has_unicode_io capability.
* wincap.h: Add has_unicode_io capability.
/* io.h
Copyright 2003 Robert Collins [EMAIL PROTECTED]
Copyright 2003 Ron Parker [EMAIL PROTECTED]
This file is part of Cygwin.
This software is a copyrighted work licensed under the terms of the
Cygwin license. Please consult the file CYGWIN_LICENSE for
details. */
#ifndef _IO_H_
#define _IO_H_
/* this file is a tuhnk layer to automatically use unicode calls when possible.
* ALL routines presume that valid memory checks have already been carried out.
*/
inline bool cygwin_filename_is_dos(LPCTSTR file_name)
{
return file_name[1] == ':';
}
class IOThunkState
{
public:
inline IOThunkState(LPCTSTR file_name);
inline ~IOThunkState();
enum Condition {FAILED, ANSI, WIDE} condition;
inline WCHAR *getWide();
/* Not implemented */
inline IOThunkState(IOThunkState const );
inline IOThunkState operator = (IOThunkState const );
private:
size_t len;
WCHAR *wfilename;
LPCTSTR file_name;/* pointer used during object life */
};
IOThunkState::IOThunkState(LPCTSTR filename): wfilename (NULL), file_name(filename)
{
len = strlen(file_name);
/* If it exceeds ANSI call length, fail. */
if (!wincap.has_unicode_io() len MAX_PATH)
{
SetLastError(111); /* The file name is too long. */
condition = FAILED;
return;
}
/* Call the ansi call if possible / required */
if (!wincap.has_unicode_io() || !cygwin_filename_is_dos(file_name)
|| len = MAX_PATH)
{
condition = ANSI;
return;
}
/* we need to use unicode to support this call */
getWide();
}
WCHAR *
IOThunkState::getWide()
{
if (wfilename != NULL || condition == FAILED)
return wfilename;
if ((wfilename = (WCHAR *)malloc (sizeof(WCHAR) * (len + 4 + 1))) == NULL)
{
SetLastError(111); /* The file name is too long. */
condition = FAILED;
return NULL;
}
condition = WIDE;
/* And convert */
sys_mbstowcs (wfilename, ?\\, 5);
sys_mbstowcs (wfilename + 4, file_name, len + 1);
return wfilename;
}
IOThunkState::~IOThunkState()
{
if (wfilename)
free(wfilename);
}
inline HANDLE
cygwin_create_file (LPCTSTR filename, DWORD access, DWORD share_mode,
LPSECURITY_ATTRIBUTES sec_attr,
DWORD disposition, DWORD flags,
HANDLE template_file)
{
IOThunkState state(filename);
switch (state.condition)
{
case IOThunkState::FAILED:
return 0;
case IOThunkState::ANSI:
return CreateFileA (filename, access, share_mode, sec_attr, disposition,
flags, template_file);
case IOThunkState::WIDE:
return CreateFileW (state.getWide(), access, share_mode, sec_attr, disposition,
flags, template_file);
};
}
inline
BOOL cygwin_create_directory (LPCTSTR filename, LPSECURITY_ATTRIBUTES sec_attr)
{
IOThunkState state(filename);
switch (state.condition)
{
case IOThunkState::FAILED:
return 0;
case IOThunkState::ANSI:
return CreateDirectoryA (filename, sec_attr);
case IOThunkState::WIDE:
return CreateDirectoryW (state.getWide(), sec_attr);
};
}
inline
DWORD cygwin_get_file_attributes(LPCTSTR filename)
{
IOThunkState state(filename);
switch (state.condition)
{
case IOThunkState::FAILED:
return INVALID_FILE_ATTRIBUTES;
case IOThunkState::ANSI:
return GetFileAttributesA (filename);
case IOThunkState::WIDE:
return GetFileAttributesW (state.getWide());
};
}
inline
BOOL cygwin_set_file_attributes(LPCTSTR filename, DWORD attr)
{
IOThunkState
Re: thunk createDirectory and createFile calls
On Sat, 2003-11-15 at 00:04, Max Bowsher wrote: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/base/createfile.asp Look at the grey box :}. Exactly. CreateFile takes LPCTSTR - but you are calling CreateFileA, which takes LPCSTR. Granted, LPCTSTR == LPCSTR when UNICODE is not defined - but if you are relying on that, you don't need to bother with the A suffix on the function, either. Ah, I'll look this up tomorrow. For now, I've done the change to not use MAX_PATH throughout cygwin, and I've broken something. So, I'm figuring out why :[. That said, I don't plan to rely on UNICODE not being defined: this code should be the same no matter what options are passed. Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. signature.asc Description: This is a digitally signed message part
Re: thunking, the next step
On Fri, 2003-11-14 at 23:58, Corinna Vinschen wrote: I'm wondering if we couldn't get rid of that strlen call. These functions already get a Windows path. This path is constructed by a call to path_conv::check(). check() already scans the path so it should be simple to add a length field to path_conv, which could be used when calling the IOThunkState constructor. Right? Wrong? Probably yes. Passing length in (i.e. pascal style strings :}) would be useful. I've got a short term goal of a *working* stupidly long paths cygwin first though :}. I'm not hearing any 'your concepts are stuffed' feedback though, which I like :}. Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. signature.asc Description: This is a digitally signed message part
Re: thunk createDirectory and createFile calls
On Sat, 2003-11-15 at 02:52, Christopher Faylor wrote: On Fri, Nov 14, 2003 at 08:41:21PM +1100, Robert Collins wrote: Rename CreateFile to cygwin_create_file throughout. ^^ Rename CreateDirectory to cygwin_create_directory throughout. ^^^ It is a given that we're working in cygwin. Adding a cygwin_ to the beginning of a function is just noise. Heh, will fix... was following Ron's convention semi-blindly. Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. signature.asc Description: This is a digitally signed message part
Re: For masochists: the leap o faith
Robert Collins wrote: Ok, so this it for tonight, my bed is calling me. If playing with this, be sure to: rebuild libc as well as cygwin1.dll. be setup to debug cygwin1.dll. I don't *think* I've changed the size of the shared stuff, but then again, I'm pretty tired, so I'll believe anything right now. My plan is to unbreak cygwin tomorrow, and then work through the list of potentially update-requiring API calls: Turns out, that we still have a bug with gcc, where registeres are trashed when alloca is used to allocate large stack objects. I posted a test case to the developers list when we where working on -O3 support ?two? years back - it looks like the same issue. So, I've dropped CYG_MAX_PATH to 512 in winsup.h, and that makes the dll usable. There is an issue with the win32 responses on loong files - but at least we can continue progressing. So, Chris, are there any parts you've seen so far, that you've be happy to ok (i.e. the MAX_PATH-CYG_MAX_PATH rename), or the global search and replaces to the thunk functions? As far as applications maing assumptions, unix file systems don't guarantee PATH_MAX: thats an upper limit of the OS, applications are expected to be able to handle to up to PATH_MAX... but can't expect the OS to do so in every case. Now, for global use of an A or W function, Chris' utf patch which I just ran into digging into a INVALID_NAME error, also chose at runtime. I can easily alter IOThunkState to always use W if available, and then check a cached flag from then on in. I really think that the current overhead will be low enough to be a non-issue though. Rob
Re: For masochists: the leap o faith
Christopher Faylor wrote: On Sat, Nov 15, 2003 at 07:31:42AM +1100, Robert Collins wrote: I posted a test case to the developers list when we where working on -O3 support ?two? years back - it looks like the same issue. This problem is fixed in the gcc cvs trunk. I've asked Danny and Gerrit about backporting the fix to 3.3.2. It should be trivial to do so. In fact, the test case I created back then passes now. It may be a variant on a theme though. So, Chris, are there any parts you've seen so far, that you've be happy to ok (i.e. the MAX_PATH-CYG_MAX_PATH rename), or the global search and replaces to the thunk functions? I haven't looked at anything in great detail. This is not the best possible time for me to be reviewing massive changes to cygwin, unfortunately. Ah. Well while there are a lot of textual changes, there are only a few logical changes. I'm happy to throw this into a branch and let you ponder - but I only have a few hours to actually work on it. As far as applications maing assumptions, unix file systems don't guarantee PATH_MAX: thats an upper limit of the OS, applications are expected to be able to handle to up to PATH_MAX... but can't expect the OS to do so in every case. It is fairly unusual for PATH_MAX to be many times greater than what is support by pathconf. Now, for global use of an A or W function, Chris' utf patch which I just ran into digging into a INVALID_NAME error, also chose at runtime. I can easily alter IOThunkState to always use W if available, and then check a cached flag from then on in. I really think that the current overhead will be low enough to be a non-issue though. Sorry but you've lost me. I don't know what my utf patch is. Chris January :}. For the record, I don't have any problems with changing PATH_MAX to CYG_PATH_MAX as a first step for this change. Small steps are, as always, appreciated. Alrighty, will commit that in a minute. Rob
Re: For masochists: the leap o faith
Christopher Faylor wrote: On Sat, Nov 15, 2003 at 07:31:42AM +1100, Robert Collins wrote: Robert Collins wrote: Ok, so this it for tonight, my bed is calling me. As far as applications maing assumptions, unix file systems don't guarantee PATH_MAX: thats an upper limit of the OS, applications are expected to be able to handle to up to PATH_MAX... but can't expect the OS to do so in every case. It is fairly unusual for PATH_MAX to be many times greater than what is support by pathconf. And yet: http://www.opengroup.org/onlinepubs/007908799/xsh/fpathconf.html the notes allude to the issue: unless you call [f]pathconf with a path for details on, some implementations won't supply valid information. You're right though, that we need to update [f]pathconf as part of this. If the implementation needs to use path to determine the value of name and the implementation does not support the association of name with the file specified by path, or if the process did not have appropriate privileges to query the file specified by path, or path does not exist, pathconf() returns -1 and errno is set to indicate the error. and in http://www.opengroup.org/onlinepubs/007908799/xsh/limits.h.html we have Pathname Variable Values The values in the following list may be constants within an implementation or may vary from one pathname to another. For example, file systems or directories may have different characteristics. A definition of one of the values will be omitted from the limits.h header on specific implementations where the corresponding value is equal to or greater than the stated minimum, but where the value can vary depending on the file to which it is applied. The actual value supported for a specific pathname will be provided by the pathconf() function. I think we need to remove the PATH_MAX constant as per their comment, as we will now offer runtime differences: win9X NT FAT NT NTFS. NAME_MAX will still be constant, (although I haven't reviewed msdn on that yet). Rob
Re: For masochists: the leap o faith
Christopher Faylor wrote: For the record, I don't have any problems with changing PATH_MAX to CYG_PATH_MAX as a first step for this change. Small steps are, as always, appreciated. Ok, so thats done. What about, for a next step, simply the introduction of the thunk layer - with only A calls used by it. No Unicode, no length changes. ? Thats probably the most verbose change, with the least variety in approach. From there on in we can debate the relative merits of path_conv state versus IOThunkState and the like without a huge patch in the wings. Rob
Re: For masochists: the leap o faith
On Sat, 2003-11-15 at 15:43, Christopher Faylor wrote: On Sat, Nov 15, 2003 at 09:48:46AM +1100, Robert Collins wrote: Christopher Faylor wrote: It is fairly unusual for PATH_MAX to be many times greater than what is support by pathconf. And yet: http://www.opengroup.org/onlinepubs/007908799/xsh/fpathconf.html Yes, I've already (obviously?) been to SUSv3. I wasn't talking about standards. I was talking about common practice. If you have a common practice web site that you want to show me then that might be a convincing argument. Otherwise, I'll have to fall back on my personal UNIX experience. I'm not vetoing the change because PATH_MAX is potentially large. I was kind of hoping (because I'm in incurable optimist) to start a discussion with people who were familiar with packages that used PATH_MAX. How SUSv3 defines PATH_MAX is irrelevant to existing programs. Well, it'll keep. I'll publish up my latest revision of the patch tonight, and leave it for Ron or other interested parties to carry through. There is obviously another couple of days work to get all the edges off, and then there's the gcc objects-on-stack issue to resolve. Still it'd be great to get this in, in some fashion. I would like to get the thunking changes in, simply to make the only part of the patch outstanding the controversial stuff. Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: Deep directory support
On Thu, 2003-09-25 at 02:23, Parker, Ron wrote: Any thoughts or input? Well, cygwin-patches isn't the right discussion point for this, it's reserved for patches - I'll reply, but over in [EMAIL PROTECTED] Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Checking integrity of installed packages in cygcheck
On Sun, 2003-08-10 at 02:12, Christopher Faylor wrote: On Thu, Aug 07, 2003 at 06:50:10PM -0400, Igor Pechtchanski wrote: Hi, Also some kind of functionality which would allow cygcheck to query the same files as the web search would be really cool. Something like a: cygcheck --whatprovides /usr/bin/ls.exe would be really useful. Hmm, I think we're getting into stuff that setup should do itself. We -do- have command line functionality... Another interesting thing would be to do some ntsec/mkpasswd/mkgroup type sanity checks or even to fix up common ntsec problems. That sounds good for cygcheck. Cheers, Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Checking integrity of installed packages in cygcheck
On Sun, 2003-08-10 at 10:12, Christopher Faylor wrote: I don't see how that will ever be possible given the windows problems with stdio and GUI apps. I guess we could make setup a console utility but that would result in the ugly black console box flashing up whenever you start setup.exe. I was thinking of something like 'cygsetup' a console only version. cygcheck could then call that for the setup-related stuff it does now, and these extra features. We still have teasing apart of the GUI and logic components to finish before this becomes easy to do... Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [ford@vss.fsi.com: [PATCH] Trivial pthread testsuite fixes]
On Thu, 2003-03-27 at 18:45, Thomas Pfaff wrote: Christopher Faylor wrote: I assume that these patches are to clean up some warnings. Do they make sense? Yes. I have recognized these warnings yesterday too and will apply the patch. Thank you. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] The great pthread rename
On Thu, 2003-03-27 at 22:11, Thomas Pfaff wrote: Do not try to force me to comment every bit of this patch ;-) 2003-03-27 Thomas Pfaff [EMAIL PROTECTED] * thread.h: Change class names, methods, members and local vars according to the GNU coding style. * thread.cc: Ditto. * dcrt0.cc (dll_crt0_1): Rename pthread::initMainThread call to pthread::init_mainthread. * pthread.cc (pthead_getsequence_np): Rename pthread::isGoodObject call to pthread::is_good_object. Reminds me why I don't require the GNU standards for setup.exe :}. Thanks, and please apply. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Change pthread equations
On Thu, 2003-03-27 at 23:52, Thomas Pfaff wrote: 2003-03-27 Thomas Pfaff [EMAIL PROTECTED] * thread.cc: Change 1==foo equations to foo==1 throughout. Thanks again - please apply. I don't know if you meant to do this: - return (pthread_equal ((*mutex)-owner, self)) 1 == (*mutex)-recursion_counter; + return ((*mutex)-recursion_counter == 1 pthread_equal ((*mutex)-owner, self)); But it's actually more than just changing 1==foo to foo==1. If there where side effects in pthread_equal, the meaning would have changed. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: Patched doc/setup-net.sgml
On Sun, 2003-03-23 at 11:55, Igor Pechtchanski wrote: On 23 Mar 2003, Robert Collins wrote: [snip] * For most users, the Direct Connection method of downloading is the best choice. IMO the IE5 method is best. I've been considering making it the default. The IE5 method will leverage your IE5 cache and or organisational proxy server for performance. It will also honour browser auto-configuration scripts. This doesn't work for me. I'm still trying to figure out why, but since the possibility is there, better err on the side of caution. Mouse button scrolling doesn't work for some users. We don't disable it for them. Setup was failing to setup ntsec just recently, but we didn't err on caution there. I see no reason to err on caution here, particularly when the benefits are huge. * setup.ini setup actually downloads setup.bz2 these days, setup.ini is a suported legacy config file. I don't know if you want to mention that or not. It's stored locally as setup.ini, though, isn't it? In setups *private*, just to *change without notice* local package dir yes. User programs MUST NOT consider whats there canonical unless the authors sit on this list and track as setup changes. It's certainly not something to tell folk about during the installation! [snip] * packages are divided into categories you might want to say 'grouped into', or 'categorized by'. divided into could suggest literal division of individual packages to a non english reader - i.e. /bin files go into the foo package :]. How about Packages are assigned categories, and one package may belong to multiple categories. Also good. Possibly we need a more user orientated flavour.. Packages are grouped into categories by the author, and can be found under any of those categories in the 'categories' hierarchical chooser view ? Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] updated pthread list patch
On Thu, 2003-03-20 at 18:50, Thomas Pfaff wrote: On Wed, 19 Mar 2003, Robert Collins wrote: I just followed the already existing method names in thread.h. Methods like forEach, initMutex or isGoodObject were made by you. Yes, I realise that. I would suggest to commit my patch now and do a method renaming and 1==foo to foo==1 cleanup in a second step. Ok, please do so. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] pthread_equal
On Thu, 2003-03-20 at 18:28, Thomas Pfaff wrote: On Wed, 19 Mar 2003, Robert Collins wrote: On Thu, 2003-03-20 at 00:54, Thomas Pfaff wrote: 2003-03-19 Thomas Pfaff [EMAIL PROTECTED] * pthread.cc (pthread_equal): Replacement for pthread_equal in thread.cc. * thread.cc: Rename pthread_equal to pthread::equal throughout. (pthread_equal): Remove. * thread.h (pthread::equal): New static method. This seems mostly pointless to me. A few notes: Why use a static method? you'll always have one pthread to compare to , so using operator == is appropriate. In fact, operator == already does the right thing as it is the entire contents of pthread_equal. So: where pthread_equal is used internally, you could switch to (for instance) == if (thread == joiner) == The only reason for this patch is to give the compiler the opportunity to do some inline optimizations. Without it it will always issue a function call only to test for equality of two pointers. Huh? Not if you use the operator == syntax it won't. The synthetic operator == is always inlined. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] reorganize list handling of fixable pthread objects
On Fri, 2003-02-28 at 22:10, Thomas Pfaff wrote: Reorganize the list handling of the pthreads objects by using the List template class and remove a lot of duplicate code. This looks good, except for for_each. can you do a proper for_each template implementation? Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] add support for PTHREAD_MUTEX_NORMAL
On Mon, 2003-03-17 at 19:17, Thomas Pfaff wrote: On Thu, 13 Mar 2003, Cygwin (Robert Collins) wrote: This: if (1 == InterlockedIncrement ((long *)lock_counter)) is not safe. You can only check for equal to 0, less than 0, and greater than 0 with InterlockedIncrement | Decrement. The xadd based inline interlocked functions in winbase.h are now enabled by default, so it is valid to test for 1 at this point. Enabled by default. Sure, as long as they aren't turned off again, or someone builds without them to get 386 support... Please, use the compatible test, it won't alter the code much. You can test for 0 and 0 safely. It looks much cleaner to me to start a counter at 0 not at -1. And the code now supports UINT_MAX instead of INT_MAX waiting threads (even if INT_MAX threads are only academicical i see no reason to add a limit here). Well there is a limit either way. I don't see any pragmatic difference. Secondly, IIRC lock_counter should be long, so the (long *) typecasting isn't needed. IMHO it should be unsigned since it makes no sense to have negative counter values. In practice it doesn't make any difference because there are not greater or smaller equations in the code. It's about type safety. Please, correct it. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] add support for PTHREAD_MUTEX_NORMAL
On Wed, 2003-03-19 at 21:15, Thomas Pfaff wrote: Enabled by default. Sure, as long as they aren't turned off again, or someone builds without them to get 386 support... Please, use the compatible test, it won't alter the code much. You can test for 0 and 0 safely. The mutex stuff does not work on i386 at all because it requires InterlockedCompareExchange. The reason to enable the Interlocked stuff in winbase.h was to keep cygwin running on Win95 that has no implementation for InterlockedCompareExchange. It simply can't be disabled without loosing support for Win95/NT3.5. All later Windows versions behave the same way than the inline ones, therefore it does not matter if the inlines were disabled again. Sure someone could only enable the inline InterlockedCompareExchange but why the hell he should do this. Ah, I had missed that i386 was impossible no matter what. Ok, that use is fine. Secondly, IIRC lock_counter should be long, so the (long *) typecasting isn't needed. IMHO it should be unsigned since it makes no sense to have negative counter values. In practice it doesn't make any difference because there are not greater or smaller equations in the code. It's about type safety. Please, correct it. Why not create an InterlockedIncrement|Decrement that takes unsigned long arguments instead ? This has nothing to do with type safety but with lack of functions. Either way, the typecast is not needed. Please either: * make the variable long * make unsigned variants of the interlockedIncrement|Decrement that will throw (not C++, rather a processor exception) overflow or underflow as appropriate. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] pthread_equal
On Thu, 2003-03-20 at 00:54, Thomas Pfaff wrote: 2003-03-19 Thomas Pfaff [EMAIL PROTECTED] * pthread.cc (pthread_equal): Replacement for pthread_equal in thread.cc. * thread.cc: Rename pthread_equal to pthread::equal throughout. (pthread_equal): Remove. * thread.h (pthread::equal): New static method. This seems mostly pointless to me. A few notes: Why use a static method? you'll always have one pthread to compare to , so using operator == is appropriate. In fact, operator == already does the right thing as it is the entire contents of pthread_equal. So: where pthread_equal is used internally, you could switch to (for instance) == if (thread == joiner) == Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Add unsigned long Interlocked functions
Looks good to me. Chris, you happy with the winbase stuff? Rob On Thu, 2003-03-20 at 00:49, Thomas Pfaff wrote: 2003-03-19 Thomas Pfaff [EMAIL PROTECTED] * thread.cc (pthread_cond::Wait): Remove typecasts for unsigned long values when calling Interlocked functions. Use new UL functions instead. (pthread_mutex::_Lock): Ditto. (pthread_mutex::_TryLock): Ditto. * winbase.h (InterlockedIncrementUL): New inline function for type safety with unsigned parameters. (InterlockedDecrementUL): Ditto. (InterlockedExchangeUL): Ditto. (InterlockedCompareExchangeUL): Ditto. -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Implement pthread_rwlocks
This work looks pretty nice :}. You are using the thunk approach here - the same one another patch of yours removes is that deliberate? I'd rather not introduce more of that approach. Approval to commit is granted. Rob
Re: [PATCH] add support for PTHREAD_MUTEX_NORMAL
This: if (1 == InterlockedIncrement ((long *)lock_counter)) is not safe. You can only check for equal to 0, less than 0, and greater than 0 with InterlockedIncrement | Decrement. Secondly, IIRC lock_counter should be long, so the (long *) typecasting isn't needed. Rob
Re: [PATCH]: Updated pthread_cond patch.
I * think* this one is ok. No red flags glared at me :}. So, please, go ahead. Rob === - Original Message - From: Thomas Pfaff [EMAIL PROTECTED] To: [EMAIL PROTECTED] Sent: Thursday, February 27, 2003 11:27 PM Subject: [PATCH]: Updated pthread_cond patch. This is a slightly modified version of my still pending patch. The modifications were made to support rwlocks. 2003-02-27 Thomas Pfaff [EMAIL PROTECTED] * thread.h (pthread_cond::ExitingWait): Remove. (pthread_cond::mutex): Ditto. (pthread_cond::cond_access): Ditto. (pthread_cond::win32_obj_id): Ditto. (pthread_cond::TimedWait): Ditto. (pthread_cond::BroadCast): Ditto. (pthread_cond::Signal): Ditto. (pthread_cond::waiting): Change type to unsigned long. (pthread_cond::pending): New member. (pthread_cond::semWait): Ditto. (pthread_cond::mtxIn): Ditto. (pthread_cond::mtxOut): Ditto. (pthread_cond::mtxCond): Ditto. (pthread_cond::UnBlock): New method. (pthread_cond::Wait): Ditto. * thread.cc: Update list of cancellation points. (pthread_cond::pthread_cond): Rewrite. (pthread_cond::~pthread_cond): Ditto. (pthread_cond::TimedWait): Remove. (pthread_cond::BroadCast): Ditto. (pthread_cond::Signal): Ditto. (pthread_cond::UnBlock): Implement. (pthread_cond::Wait): Ditto. (pthread_cond::fixup_after_fork): Rewrite. (pthread_mutex::fixup_after_fork): Remove DETECT_BAD_APP conditional. (__pthread_cond_broadcast): Just return 0 if the condition is not initialized. Call pthread_cond::UnBlock to release blocked threads. (__pthread_cond_signal): Ditto. (__pthread_cond__dowait): Rewrite. (pthread_cond_timedwait): Add pthread_testcancel call. Fix waitlength calculation. (pthread_cond_wait): Add pthread_testcancel call.
Re: [PATCH] Remove wrapper functions in pthread.cc
On Sat, 2003-03-01 at 00:53, Thomas Pfaff wrote: This patch removes all wrapper functions in pthread.cc that only add an additional function call. Export the functions in thread.cc instead. Please apply. Cheers, Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Remove wrapper functions in pthread.cc
On Sat, 2003-03-01 at 02:29, Thomas Pfaff wrote: On Fri, 28 Feb 2003, Robert Collins wrote: On Sat, 2003-03-01 at 00:53, Thomas Pfaff wrote: This patch removes all wrapper functions in pthread.cc that only add an additional function call. Export the functions in thread.cc instead. Please apply. Impossible until you have reviewed the other patches ;-) Well, this patch is pretty straight forward to recreate :}. Up to you when you apply it though. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] a new pthread_cond implementation
On Thu, 2003-01-23 at 23:21, Thomas Pfaff wrote: 4. The spec requires that the mutex which is used with the condition shall be locked by the calling thread. The current code does not check this and will additionally create the mutex if it initialized with PTHREAD_MUTEX_INITIALIZER. The opengroup spec suggests EPERM under that condition. The spec only requires this for pthread_cond_wait, not for all pthread_cond_ calls. I hadn't noticed the EPERM suggestion. I want to go through this patch in detail - it'll be a couple of days. Please don't check it in yet. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] system-cancel part2
On Wed, 2003-01-15 at 22:23, Thomas Pfaff wrote: This patch will make sure that the signal handlers that are saved in the system call are restored even if the thread got cancelled. Since spawn_guts uses waitpid when mode is _P_WAIT spawn_guts is a cancellation point. Attached is the patch and a new test case. The new test case doesn't appear to check that the signal handlers where saved. Am I misreading that? Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] system-cancel part2
On Thu, 2003-01-16 at 00:19, Thomas Pfaff wrote: On Wed, 15 Jan 2003, Robert Collins wrote: The test case was created to prove that system is a cancellation point even if the child process is already created and the system call is waiting on child termination. Thank you for the new test cases. When Chris approves the system() changes, I'm happy with the pthreads aspects of this. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Make system a pthread cancellation point
On Tue, 2003-01-14 at 21:50, Thomas Pfaff wrote: Sorry, no testcase for that patch (it is really to simple). I think it really is worth adding test cases - even for simple things. It prevents regressions, which is the main reason for testing in the first place. So, please, if a test case can be written, lets do so. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Make sleep and usleep a cancellation point
On Fri, 2003-01-10 at 19:53, Thomas Pfaff wrote: This patch will make sleep and usleep a pthread cancellation point. Looks good to me. Please do a test case for them. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] make handle_sigsuspend a cancellation point
On Fri, 2003-01-10 at 19:57, Thomas Pfaff wrote: This patch will make handle_sigsuspend (used by pause, sigpause and sigsuspend) a pthread cancellation point. Also looks good. Again, please do a scriptable testcase for these. Rob -- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. signature.asc Description: This is a digitally signed message part
Re: [PATCH] Patch for MTinterface
On Sat, 2002-11-23 at 01:55, Thomas Pfaff wrote:
On Tue, 5 Nov 2002, Robert Collins wrote:
Overall this looks good. What happens to non-cygwinapi created threads
now though? You mention you don't agree with the code, but I can't see
(from a brief look) how you correct it.
BTW: I'm currently packing to move house, so don't expect much feedback
until late next week, or early the week after :[.
Ping
Pong. I've added the test cases to the test suite. In future please
follow the guidelines in testsuite/readme for test behaviour - running
$ ./testname || echo foo
should echo foo on failures - and neither of your test cases did that
initially.
Also, the initMainThread behaviour:
initMainThread (bool dosomething)
{
if (!dosomething)
return;
...
I don't like. I'd rather we not call initMainThread than call it with a
boolean as above.
If dosomething was internal to the pthread class, so that initMainThread
became:
initMainThread()
{
if (!dosomething)
return;
...
I'd have no issue.
Anyway, thanks again for excellent work, and the patch has been
committed.
Rob
--
---
GPG key available at: http://users.bigpond.net.au/robertc/keys.txt.
---
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] Patch for MTinterface
On Wed, 2002-11-06 at 00:14, Thomas Pfaff wrote: I have discovered some problems with the current MTinterface implementation. Here are 2 test cases: Even if the handles would be valid the pthread_join call would try to delete a thread object that is created static which would result in a corrupted heap. Ouch. Good catch. 2: fork related The forked child will not get the same thread handle as its parent, it will get the thread handle from the main thread instead. The child will not terminate because the threadcount is still 2 after the fork (it is set to 1 in MTinterface::Init and then set back to 2 after the childs memory gets overwritten by the parent). For memory that should not be copied, mark it with NO_COPY in the declaration. MTinterface is set thusly IIRC. And i do not agree with the the current pthread_self code where the threadcount is incremented if a new thread handle has been created but never gets decremented (i do not expect that threads that are not created by pthread_created will terminate via pthread_exit). And the newly created object never gets freed. The dllinit routine will take care of this when we get that implemented again. I don't To avoid these errors i have made changes that will create the mainthread object dynamic and store the reents and thread self pointer via fork safe keys. Overall this looks good. What happens to non-cygwinapi created threads now though? You mention you don't agree with the code, but I can't see (from a brief look) how you correct it. BTW: I'm currently packing to move house, so don't expect much feedback until late next week, or early the week after :[. Rob -- --- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. --- signature.asc Description: This is a digitally signed message part
Re: [PATCH] Modified pthread types
On Mon, 2002-11-04 at 15:28, Christopher Faylor wrote: I'm going through my mailbox cleaning up old patches. AFAICT, this patch wasn't applied. Was there a reason for that? Yep, It got lost :[. I'm not happy as-is, but have put it in my holding folder now, so as soon as I have a little more time, I will apply a variant thereof. Rob -- --- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. --- signature.asc Description: This is a digitally signed message part
Re: [PATCH] fix segv in pthread_mutex::init
On Thu, 2002-10-17 at 18:11, Thomas Pfaff wrote: This patch should fix the segfault in pthread_mutex::init by changing the test order for a valid object and checking for valid initializer object first.. I'm happy with the verifyable_object change. I'm not happy with the pthread_mutex::init change (yet). I've checked in the verifyable_object stuff, along with a pthread_mutex::init change I am happy with. The reason I wasn't happy with your change was threefold: 1) there is no need to add scoping to static calls within a class. Doing so reduces readability, and should be avoided. 2) The test was no longer *obvious* at first read, whereas (IMO) what I've checked in is. Cheers, Rob -- --- GPG key available at: http://users.bigpond.net.au/robertc/keys.txt. --- signature.asc Description: This is a digitally signed message part
Re: [PATCH] pthread key destructor
On Mon, 2002-09-23 at 22:34, Thomas Pfaff wrote: See http://www.opengroup.org/onlinepubs/007904975/functions/pthread_key_create.html I do not think that we should support more than one iterations at the moment. This seems to be a rather new addition to the pthread specification. Hmm, I recall that being there for quite a while. But yes, I agree for now, we won't worry. Your patch was also slightly incorrect - we are meant to reset the value *before* running the destrcutor. So I've implemented such a version. Thanks. Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] Reset threadcount after fork
On Mon, 2002-09-23 at 22:36, Thomas Pfaff wrote: 2002-09-23 Thomas Pfaff [EMAIL PROTECTED] * thread.cc (MTinterface::fixup_after_fork): Reset threadcount to 1 after fork. Why do we need this? MTinterface::Init is also called after fork, and sets threadcount to 1. Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] new mutex implementation 2. posting
On Mon, 2002-09-23 at 22:45, Thomas Pfaff wrote: I said that my implementation works similar as critical sections (or Chris mutos). Oh, I must have misread. Sorry. Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] new mutex implementation 2. posting
On Fri, 2002-09-20 at 22:43, Thomas Pfaff wrote: On Fri, 20 Sep 2002, Robert Collins wrote: On Tue, 2002-09-17 at 19:34, Thomas Pfaff wrote: Thomas, the patch is incomplete. pthread_cond::TimedWait needs updating as well... Yup, but it seems that this was broken on NT before i made my changes, because it was never updated to use Critical Sections when they are available. Uhmm, it was working for me :}. anyway, if you can make that consistent, I will apply the semaphore based mutex code. I'm not 100% behind it, I think we need to benchmark it, but lacking the facilities, I'm going to accept it and tune later. also, please diff against current HEAD, the previous patch failed on the mutex section (I'm not sure why, may be white space changes or something). Must wait until tomorrow. I will also recreate my pending patches 3 and 4 against current since your your patch has broken some parts of them. Lets talk about those a little first. I'll email separately. Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] pthread_fork Part 3
On Sat, 2002-08-17 at 06:55, Thomas Pfaff wrote: Pthread key destructor handling revised. IMHO it does not make sense to handle two lists with keys, one with all keys, one with its destructors. The destructors are now part of the key class. I agree with the duplication of code. This is one area I'd really really really like to use templates. Chris, Corinna, if we ever get the chance to use templates please tell me so! It makes code clarity and size so much better. Anyway, yes, we should only have one list. So yes, please do refactor the two together in the way I've arranged the pthread_keys::keys list. Note that you have a comment on non thread safeness in the new pthread_keys code. I thought I had addressed that in my list code, could you either tell me I was also not thread safe, or correct that at the same time? Cheers, Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] pthread_fork Part 3
On Fri, 2002-09-20 at 23:06, Thomas Pfaff wrote:
If you want to work around this you must use a mutex to protect the entire
list.
Or: don't delete foo; the keys, instead foo-deleteme();
pthread_key::pthread_key(){
inuse_count = 1;
}
pthread_key::deleteme() {
interlockedincrement(inuse_count)
if interlockeddecrement (inuse_count) == 0
delete this;
}
pthread_key::rundestructor() {
if (interlockeddecrement(inuse_count) == 0)
delete this
}
This prevents the race you describe with no locks.
Still, this race is actually one ieee says we don't care about IIRC,
it's up to the user to synchronise calls like this.
Rob
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] pthread_fork Part 1
On Tue, 2002-09-17 at 18:48, Thomas Pfaff wrote: I have attached a small source file for testing. Thanks. I've commited my reworked version. My main goal was to get a working threaded perl, so this was the reference source for the final testing. With all patches applied (and the changed mutex implementation) i was able to build and run it without problems. Cool. Well I'm onto the fork(2) patch. Re: the mutex implementation. Can you gather all the mutex patchs you sent (there where what, 3?) and provide a single bug-free-as-far-as-you-know version? If, and only if, it makes sense for me to review the patches sent for the mutex previously, then I will. I know I wasn't happy with them at that point, mainly because of things I think you've already address'd... Cheers, Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] pthread_fork Part 2
On Sat, 2002-08-17 at 06:32, Thomas Pfaff wrote:
Some small fixes in the pthread key handling.
-1020,16 +1020,27 pthread_key::~pthread_key ()
int
pthread_key::set (const void *value)
{
- /*the OS function doesn't perform error checking */
- TlsSetValue (dwTlsIndex, (void *) value);
+ if (dwTlsIndex == TLS_OUT_OF_INDEXES ||
Not needed. dwTlsIndex is not set by anyone outside the class, AND
if TlsAlloc fails, then we set the magic to 0, causing a failure on
creation.
Are you covering the situation where the restoreFromSavedBuffer call
fails? If so, then we should cause the object to destroy itself in that
call, thus causing the VALID_OBJECT test to fail for future calls from
userland.
+ !TlsSetValue (dwTlsIndex, (void *) value))
Please see the MS documentation on this. They explicitly state that they
perform minimal checking. Also, this should be an assert, as TlsSetValue
can only fail if you give it an invalid index, and our index is assigned
by the OS.
+ if (dwTlsIndex == TLS_OUT_OF_INDEXES)
Ditto to above.
+result = TlsGetValue (dwTlsIndex);
And again.
void
-1884,8 +1895,8 __pthread_setspecific (pthread_key_t key
{
if (verifyable_object_isvalid (key, PTHREAD_KEY_MAGIC) != VALID_OBJECT)
return EINVAL;
- (key)-set (value);
- return 0;
+
+ return (key)-set (value);
Not needed, because of the above lack of changes.
Yes, what you are suggesting is good general programming practice, but
these are performance critical functions, and we are checking for a
situation that can't happen short of someone writing into our memory
space. If that happens, errors are the least of our problems :}.
Rob
signature.asc
Description: This is a digitally signed message part
RE: Patch to pass file descriptors
-Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Conrad Scott Sent: Friday, 28 June 2002 9:00 PM To: [EMAIL PROTECTED] Subject: Re: Patch to pass file descriptors Corinna Vinschen [EMAIL PROTECTED] wrote: More problematic is the approach to use cygserver for this. I've talked to Chris about passing descriptors and we agree in that we want to try under all circumstances to find a solution which doesn't need cygserver. Corinna, I thought that the main reason to use cygserver for this is for security reasons. Your final paragraph mentions this issue but it's not clear whether it's a complete solution (and I'm not fully up to speed on the NT security model, so I've no idea). One issue tho' is that you'll have to create the shared memory segment with global read (and write) permissions since you've no idea of the security level of the receiving process. If the sender then puts its process handle, with the PROCESS_DUP_HANDLE privilege, into that shared memory, any process on the system can read the shared memory and now has access to *all* of the sender's handles (i.e., just run through all the small integers running DuplicateHandle on them). You could put some obfuscation into the system by generating random names for the shared memory segment but that's still not ideal. Yes. I was just about to write up this problem. It's *exactly* the same issue that exists with vty's and that Egor's original daemon proof-of-concept was designed to correct. Chris/Corinna, why do you want to avoid *new* functionality (especially with security complications) using the cygserver? Also, I think that the proposed approach will have performance issues as the receiving process may not read from the socket for an indefinite time period, and isn't known in advance, so cannot be alerted, and finally the call has to block to prevent the process terminating and closing the handles before they are received. Rob
RE: Pthreads patches
Thanks Thomas, this is good. I'm checking it in now. Rob -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Thomas Pfaff Sent: Tuesday, 18 June 2002 8:12 PM To: [EMAIL PROTECTED] Subject: Re: Pthreads patches I am sorry, but i recognized that my patch was incomplete. The diff included only threads.cc. I have attached a new one. Thomas 2002-06-12 Thomas Pfaff [EMAIL PROTECTED] * thread.h (pthread::cleanup_stack): Renamed cleanup_handlers to cleanup_stack. * thread.cc (pthread::pthread): Ditto. (pthread::create): Fixed mutex verification. (pthread::push_cleanup_handler): Renamed cleanup_handlers to cleanup_stack. Mutex calls removed, used InterlockedExchangePointer instead. (pthread::pop_cleanup_handler): Renamed cleanup_handlers to cleanup_stack. (pthread::pop_all_cleanup_handlers): Ditto. (__pthread_once): Check state first and return if already done. (__pthread_join): DEADLOCK test reverted to __pthread_equal call. (__pthread_detach): Unlock mutex before deletion. Robert Collins wrote: I'll review this latest patch in ~20 hours. (i.e. tomorrow night). Rob
RE: Resubmission of cygwin_daemon patch.
Ok, I've got some feedback for you... I need to have a good think about some of what's being presented. The following things are unconditionally good: The pure virtual transport changes The recoverable approach, and instance detection changes. (actually, I'd like to suggest a global mutex be owned and tested against rather than checking for the socket being present. But that's orthogonal). Command line help I'll be extracting the above and committing to head sometime shortly after 1.3.11 gets released. Or if that doesn't happen within a week, then soon anyway :}. On the thoughtful side: There seems to be a lot of code duplication - definitions copied to make private versions, that sort of thing. Can you elaborate on why? I strongly prefer to only have one instance of such things to prevent skew occuring. Why have you removed the __OUTSIDE_CYGWIN__ for cygserver_shm.cc ? That's all for now, gotta run - sorry. Rob
Re: Resubmission of cygwin_daemon patch.
- Original Message - From: Conrad Scott [EMAIL PROTECTED] To: Robert Collins [EMAIL PROTECTED] Cc: [EMAIL PROTECTED] Sent: Sunday, June 23, 2002 7:35 PM About instance detection: you're right that something better could be done here. What I've ended up with is really a security patch: it's possible for another process to create an instance of a named pipe, wait for clients to connect and then impersonate them. It will always be possible to do that. Anyone can build the cygserver and insert hostile code into their build. Code interception is a standard technique for reverse engineering, runtime patching and the like. In terms of preventing someone hostilely opening the same socket/pipe, I'd have thought windows prevented multiple listening pipes with the same name. Rob
RE: Pthreads patches
-Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Thomas Pfaff Sent: Wednesday, 12 June 2002 9:17 PM To: [EMAIL PROTECTED] Subject: Re: Pthreads patches Hi Rob, i had a minor problem with your latest code: You decided to change the mutex pointer into an object. This will break the verifyable_object_isvalid call in pthread::create. I thought I might explain why. The usual benefit of an object pointer in C++ is for optional objects, or for polymorphic objects. Neither applied in this case, so a composed object is a lower overhead option. Rob
RE: Pthreads patches
I'll review this latest patch in ~20 hours. (i.e. tomorrow night). Rob
RE: [PATCH] added locks in pthread code
-Original Message- From: Thomas Pfaff [mailto:[EMAIL PROTECTED]] Sent: Monday, 10 June 2002 4:48 PM To: Robert Collins Cc: [EMAIL PROTECTED] Subject: RE: [PATCH] added locks in pthread code I wanted to make sure that a thread can not be cancelled asynchronous when it is in the cleanup push call, but i think it could be done better with InterlockedExchangePointer. Yes. Look at the destructor code, or the pthread_atfork list code - there is a thread safe list based around InterlockedExchangePointer. Rob
RE: [PATCH] added locks in pthread code
-Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Robert Collins Sent: Monday, 10 June 2002 6:41 PM To: 'Thomas Pfaff' Cc: [EMAIL PROTECTED] Subject: RE: [PATCH] added locks in pthread code -Original Message- From: Thomas Pfaff [mailto:[EMAIL PROTECTED]] Sent: Monday, 10 June 2002 4:48 PM To: Robert Collins Cc: [EMAIL PROTECTED] Subject: RE: [PATCH] added locks in pthread code I wanted to make sure that a thread can not be cancelled asynchronous when it is in the cleanup push call, but i think it could be done better with InterlockedExchangePointer. Yes. Look at the destructor code, or the pthread_atfork list code - there is a thread safe list based around InterlockedExchangePointer. Or, call setcancelstate twice during your pthread_cleanup_push macro. Once to set PTHREAD_CANCEL_ENABLE, and once to restore the old value. That avoids the issue completely, and is in spec with P1003.1. Rob
RE: [PATCH] pthread_join fix
Applied. Thanks, Rob -Original Message- From: Thomas Pfaff [mailto:[EMAIL PROTECTED]] Sent: Wednesday, 24 April 2002 8:18 PM To: [EMAIL PROTECTED] Subject: [PATCH] pthread_join fix Rob, this is an incremental update to my pthread patches. It will fix a problem when a thread is joined before the creation completed. BTW, i have not added any locks yet (the actual implementation had no), but IMHO they are required in the exit,join,cancel code. I will add locks if you agree. Greetings, Thomas 2002-04-24 Thomas Pfaff [EMAIL PROTECTED] * thread.cc (thread_init_wrapper): Check if thread is alreay joined (__pthread_join): Set joiner first. (__pthread_detach): Ditto.
RE: [PATCH] added locks in pthread code
I'm applying a variation on this. Again, mainly OOP style changes, but also making the mutex an instance rather than pointer. (And where you aware that you where leaking the mutex?) Rob -Original Message- From: Thomas Pfaff [mailto:[EMAIL PROTECTED]] Sent: Thursday, 25 April 2002 7:33 PM To: [EMAIL PROTECTED] Subject: [PATCH] added locks in pthread code The patch will add locks via mutex around critical code to protect against race conditions and fix __pthread_detach to cleanup when thread has already terminated. This an incremental update again. Greetings, Thomas 2002-04-25 Thomas Pfaff [EMAIL PROTECTED] * thread.h (pthread::mutex): new member * thread.cc (pthread::pthread): Set mutex to NULL. (pthread::~pthread): Destroy mutex. (pthread::create): Initialize mutex. (thread_init_wrapper): Protect against race. (__pthread_cleanup_push): Ditto. (__pthread_exit): Ditto. (__pthread_join): Ditto (__pthread_detach): Protect against race and cleanup if thread has already terminated.
RE: sem_getvalue patch
Ok, well I'll hold off for the assignment. Rob -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Robert Collins Sent: Saturday, 8 June 2002 2:44 AM To: 'Robb, Sam'; [EMAIL PROTECTED] Subject: RE: sem_getvalue patch Thanks, this looks good, I'll do a closer review in the weekend. Rob -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Robb, Sam Sent: Saturday, 8 June 2002 2:35 AM To: [EMAIL PROTECTED] Subject: RE: sem_getvalue patch [original message was to [EMAIL PROTECTED]] With a little effort, I've managed to build a cygwin1.dll that exports sem_getvalue(). The version of cygwin1.dll that I built seems subtly hosed, though - while I can compile and run my test program from within a Windows cmd.exe shell, trying to run bash or ls (and probably a great many other things) hangs. Here's the patch... fairly straightforward, if I've understood the SUS spec for the function correctly :-/ As for the apparent hangs in bash/ls/etc. - well, perhaps it was my patch, perhaps not, as I was building from latest cvs source. Since I can't find any documentation that indicates if a particular method for adding an export to cygiwn.din needs to be followed, this patch simply tacks sem_getvalue to the end of the list. Thanks, -Samrobb winsup/cygwin/ChangeLog entry: 2002-06-06 Sam Robb [EMAIL PROTECTED] * pthread.cc (sem_getvalue): New function. * thread.cc (__sem_getvalue): Diito. * thread.h (__sem_getvalue): Ditto. * include/semaphore.h (sem_getvalue): Ditto. * posix.sgml: Add sem_getvalue to Synchronization section. * cygwin.din: Add symbol for sem_getvalue(). winsup/doc/ChangeLog entry: 2002-06-06 Sam Robb [EMAIL PROTECTED] * calls.texinfo: Remove 'unimplemented' tag from sem_getvalue.
RE: sem_getvalue patch
Thanks, this looks good, I'll do a closer review in the weekend. Rob -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Robb, Sam Sent: Saturday, 8 June 2002 2:35 AM To: [EMAIL PROTECTED] Subject: RE: sem_getvalue patch [original message was to [EMAIL PROTECTED]] With a little effort, I've managed to build a cygwin1.dll that exports sem_getvalue(). The version of cygwin1.dll that I built seems subtly hosed, though - while I can compile and run my test program from within a Windows cmd.exe shell, trying to run bash or ls (and probably a great many other things) hangs. Here's the patch... fairly straightforward, if I've understood the SUS spec for the function correctly :-/ As for the apparent hangs in bash/ls/etc. - well, perhaps it was my patch, perhaps not, as I was building from latest cvs source. Since I can't find any documentation that indicates if a particular method for adding an export to cygiwn.din needs to be followed, this patch simply tacks sem_getvalue to the end of the list. Thanks, -Samrobb winsup/cygwin/ChangeLog entry: 2002-06-06 Sam Robb [EMAIL PROTECTED] * pthread.cc (sem_getvalue): New function. * thread.cc (__sem_getvalue): Diito. * thread.h (__sem_getvalue): Ditto. * include/semaphore.h (sem_getvalue): Ditto. * posix.sgml: Add sem_getvalue to Synchronization section. * cygwin.din: Add symbol for sem_getvalue(). winsup/doc/ChangeLog entry: 2002-06-06 Sam Robb [EMAIL PROTECTED] * calls.texinfo: Remove 'unimplemented' tag from sem_getvalue.
RE: Cleanup of ntdll.h
-Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Corinna Vinschen Sent: Thursday, 30 May 2002 5:57 PM To: cygpatch Subject: Cleanup of ntdll.h Hi, I've cleaned up the usage of NTDLL.DLL functions slightly. For some reason the function ZwQueryInformationProcess has been additionally defined even though NtQueryInformationProcess was already available. Additional functions have been defined as ZwXXX while formerly already defined functions did use the NtXXX style. I changed that now so that all functions are called with Nt prefix. This is incorrect. BOTH definitions should exist. NtXXX are kernel mode calls, ZwXXX are user mode calls that gate through to kernel mode calls via int 2eh. Rob
RE: Cleanup of ntdll.h
-Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] On Behalf Of Corinna Vinschen Sent: Thursday, 30 May 2002 6:23 PM To: cygpatch Subject: Re: Cleanup of ntdll.h On Thu, May 30, 2002 at 06:04:19PM +1000, Robert Collins wrote: [mailto:[EMAIL PROTECTED]] On Behalf Of Corinna Vinschen I changed that now so that all functions are called with Nt prefix. This is incorrect. BOTH definitions should exist. NtXXX are kernel mode calls, ZwXXX are user mode calls that gate through to kernel mode calls via int 2eh. Nope. From user mode both call types are identical. There's no need to use both forms in Cygwin and since the header is only used inside of Cygwin there's also no need to define both variations. One is enough. It's all one to me if it's the Zw or Nt version but we should at least use always the same. It was my understandinf that the NtXXX calls cannot be used from user mode. We should be using the Zw calls. Rob
RE: [PATCH] Fixed race in __pthread_mutex_init
-Original Message- From: Thomas Pfaff [mailto:[EMAIL PROTECTED]] Sent: Friday, April 26, 2002 7:08 PM To: [EMAIL PROTECTED] Subject: [PATCH] Fixed race in __pthread_mutex_init __pthead_mutex_init had a race condition if the mutex is initialized with PTHREAD_MUTEX_INITIALIZER (the mutex could be initialized by two threads simultanously). The patch wraps a mutex around mutex creation. This is will be the last patch for a while. The feedback for the previous ones was a little low, i do not know if no one (except Rob) is interested in pthreads for cygwin or my patches are not welcome. I will wait for comments now. I'm the pthread maintainer, so you only need my interest :}. I'm reviewing your other patches at the moment.. I'll have some feedback for you before too much longer. Rob
RE: Packaging information
-Original Message- From: Charles Wilson [mailto:[EMAIL PROTECTED]] Sent: Wednesday, April 24, 2002 3:58 PM To: [EMAIL PROTECTED] Subject: Re: Packaging information On Wed, Apr 24, 2002 at 12:37:24AM -0400, Charles Wilson wrote: Permission to apply? AFAIAC, the setup.html is ok but I don't think that the generic-whatever stuff belongs on the web site. Maybe the ftp directory or the cygwin-apps repository is more appropriate. I think generic-* are just as much documentation as they are resource; so the reader should be able to click on a link(*) Why not link to them via cvsweb? Rob
RE: [PATCH] dtors run twice on dll detach (update)
-Original Message- From: Robert Collins Sent: Saturday, April 20, 2002 10:18 AM To: Robert Collins; [EMAIL PROTECTED] Subject: RE: [PATCH] dtors run twice on dll detach (update) -Original Message- From: Robert Collins Sent: Saturday, April 20, 2002 8:05 AM Ookay. I don't think that either function is obsolete... and neither you nor Corinna had commented. I should enlarge on this. The reason that I don't think that either function is obsolete is as follows: Once trigger is via atexit - when the program exits. The other is at dll detachment. Now the double-dtor run does not occur under gdb or strace. This suggests to me that the dll detachment does not occur in these situations (or that atexit does not run). Also, atexit will call all the dtors before any dll's detach, which could be important. So that should stay. Conversely, dlopened dll's should have their dtors called when they are dlclosed, so the dll_detach invocation should stay. So... as this has been contentious: Chris/Corinna - any objection to my recommitting it? Rob
RE: [PATCH] dtors run twice on dll detach (update)
-Original Message- From: Christopher Faylor [mailto:[EMAIL PROTECTED]] Sent: Thursday, April 25, 2002 1:07 PM To: [EMAIL PROTECTED] Subject: Re: [PATCH] dtors run twice on dll detach (update) On Thu, Apr 25, 2002 at 12:34:33PM +1000, Robert Collins wrote: So... as this has been contentious: Chris/Corinna - any objection to my recommitting it? No: On Fri, Apr 19, 2002 at 10:42:21AM -0400, Christopher Faylor wrote: *If one of the functions is obsolete, it should be deleted. That means *that the patch does *not* look good. It needs to be reviewed. I replied to that explaining why I don't think *either* function is obsolete. http://cygwin.com/ml/cygwin-patches/2002-q2/msg00074.html Rob
RE: [PATCH] dtors run twice on dll detach (update)
-Original Message- From: Christopher Faylor [mailto:[EMAIL PROTECTED]] Sent: Thursday, April 25, 2002 1:10 PM To: [EMAIL PROTECTED] Subject: Re: [PATCH] dtors run twice on dll detach (update) On Wed, Apr 24, 2002 at 11:06:32PM -0400, Christopher Faylor wrote: On Thu, Apr 25, 2002 at 12:34:33PM +1000, Robert Collins wrote: So... as this has been contentious: Chris/Corinna - any objection to my recommitting it? No: Sorry. Make that Yes. I have the same objections that I did before. I'll review the patch in the next couple of days. Ok, cool thanks. Rob
RE: Packaging information
-Original Message- From: Charles Wilson [mailto:[EMAIL PROTECTED]] Sent: Thursday, April 25, 2002 1:24 AM Hey, yeah -- that'll work. Where should generic-* go -- in one of the existing repositories under cygwin-apps (probably not), or should I create another? If I should create another, what should it be called? resources? Hmm. Lets be a bit more specific - why not call the module packaging. Then the generics can go in /templates or /samples or something like that. Rob
RE: [PATCH]setup.exe mklink2.cc some function arguments need to be pointers
-Original Message- From: Gerrit P. Haase [mailto:[EMAIL PROTECTED]] Sent: Monday, April 22, 2002 4:34 PM To: Robert Collins Cc: [EMAIL PROTECTED] Subject: Re: [PATCH]setup.exe mklink2.cc some function arguments need to be pointers Hallo Robert, It looks like you are building in a branch. The version of mklink2.cc I have from the main branch is No, I've realised that I haven't committed it... I've some smei-invasive changes I've been mulling over in HEAD, and I'd forgotten about that. I will commit my patch to HEAD along with a couple of other fixes from setup200202 shortly - hopefully this weekend. Until then, simply use the mklink2.cc from the setup200202 branch. Is it in now? I cannot build because mklink2.cc fails... Use this: cvs -z3 update -Pdrsetup200202 mklink2.cc to get a copy that will build. Rob
RE: [PATCH]setup.exe mklink2.cc some function arguments need to be pointers
Update your win32api - And it should not need the patch, Rob -Original Message- From: Michael A Chase [mailto:[EMAIL PROTECTED]] Sent: Friday, April 19, 2002 7:26 AM To: [EMAIL PROTECTED] Subject: [PATCH]setup.exe mklink2.cc some function arguments need to be pointers I couldn't get mklink2.cc to compile until I made the attached changes. It appears that CoCreateInstance() and sl-lpVtbl-QueryInterface() are looking for pointers to values in certain arguments instead of the values. Almost the exact same code is used in src/winsup/cygwin/shortcut.c except for the ''s and it compiles cleanly. Both functions defined in mklink2.cc are declared extern C so the function calls should work the same. Here are the relevant pieces of code. src/winsup/cinstall/mklink2.cc (make_link_2): 23: CoCreateInstance (CLSID_ShellLink, NULL, 24: CLSCTX_INPROC_SERVER, IID_IShellLink, (LPVOID *) sl); 25: sl-lpVtbl-QueryInterface (sl, IID_IPersistFile, (void **) pf); src/winsup/cygwin/shortcut.c (check_shortcut): 85: hres = CoCreateInstance (CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, 86:IID_IShellLink, (void **)psl); 87: if (FAILED (hres)) 88: goto close_it; 89: /* Get a pointer to the IPersistFile interface. */ 90: hres = psl-lpVtbl-QueryInterface (psl, IID_IPersistFile, (void **)ppf); src/winsup/w32api/include/objidl.h: 640: EXTERN_C const IID IID_IPersistFile; src/winsup/w32api/include/olectlid.h: 76: extern const GUID IID_IPersistFile; src/winsup/w32api/include/shlguid.h: 13: extern const GUID CLSID_ShellLink; 28: extern const GUID IID_IShellLinkA; 68: #define IID_IShellLink IID_IShellLinkA src/winsup/w32api/lib/shell32.c: 6: DEFINE_SHLGUID(CLSID_ShellLink,0x00021401L,0,0); 21: DEFINE_SHLGUID(IID_IShellLinkA,0x000214EEL,0,0); src/winsup/w32api/lib/uuid.c: 226: DEFINE_GUID(IID_IPersistFile,0x10b,0,0,0xc0,0,0,0,0,0,0,0x46); -- Mac :}) ** I normally forward private questions to the appropriate mail list. ** Ask Smarter: http://www.tuxedo.org/~esr/faqs/smart- questions.html Give a hobbit a fish and he eats fish for a day. Give a hobbit a ring and he eats fish for an age. ChangeLog: 2002-04-18 Michael A Chase [EMAIL PROTECTED] * mklink2.cc (check_shortcut): Change arguments from values to pointers.
RE: [PATCH] minor pthread fixes
-Original Message- From: Jason Tishler [mailto:[EMAIL PROTECTED]] Sent: Thursday, April 18, 2002 9:58 PM To: Robert Collins Cc: Thomas Pfaff; [EMAIL PROTECTED] Subject: Re: [PATCH] minor pthread fixes Rob, On Thu, Apr 18, 2002 at 09:31:15PM +1000, Robert Collins wrote: Regarding 2:, again a good catch. Could #2 have caused the problem that we saw with Python's test_threadedtempfile regression test? I don't think so. python isn't creating and killing very short lived threads is it? ROb
RE: [PATCH]setup.exe mklink2.cc some function arguments need to be pointers
Well if you recall I had the opposite code in place (as far as I can tell without an actual patch), and that didn't compile for a different set of users. I completely rebuild my OS the other day, and after that I've needed the patch. That seemed a strong indication that the w32api was the culprit.. Rob -Original Message- From: Brian Keener [mailto:[EMAIL PROTECTED]] Sent: Friday, April 19, 2002 8:46 AM To: [EMAIL PROTECTED] Subject: Re: [PATCH]setup.exe mklink2.cc some function arguments need to be pointers Not to be a pain about this - but this have been reported several times in the past and I am running Win2000 and have the W32api-1.3-2 installed. I haven't seen any other w32api come down the pike so that appears to be the most recent. I have the patch - just took it out and no compile - put it back and it does compile. Not sure what Mike's OS or yours Robert or if it even makes a difference but I thought I would point out mine is Win2k. I also have my just updated my CVS for cinstall so it is current. I know this is a me2 but I thought I would add what I could. Bk
RE: [PATCH]setup.exe: delete called for NULL pointer
-Original Message- From: Michael A Chase [mailto:[EMAIL PROTECTED]] Sent: Thursday, April 11, 2002 7:51 AM To: Pavel Tsekov Cc: [EMAIL PROTECTED] Subject: Re: [PATCH]setup.exe: delete called for NULL pointer I don't know, but many other places where it is called, it's protected by if (var) delete[] var; That was before I had read up on certain aspects of the C++ spec. Rob
RE: Patch for Setup.exe problem and for mklink2.cc
-Original Message- From: Ton van Overbeek [mailto:[EMAIL PROTECTED]] Sent: Thursday, March 28, 2002 1:03 AM To: [EMAIL PROTECTED]; [EMAIL PROTECTED] Cc: [EMAIL PROTECTED] Subject: Patch for Setup.exe problem and for mklink2.cc Found the problem causing the segment violation and probably causing Jonas Eriksson's problem. It is a typical case of 'off by 1'. In PickView::set_headers the loop filling the window header does one iteration too much, resulting in a call to DoInsertItem with a NULL string pointer and hence a crash following. While debugging this I could not compile the new mklink2.cc ( the c++ version of the original mklink2.c). It seems three (address of c++ operator) have disappeared in the transition. Putting them back made the compiler happy. Is this OK Robert ? I'll check the off-by-one fix in tomorrow, as I'm off to bed now. As for the 's, I wonder if it's a w32api reference issue? The compiler complains if they are present for me. I have the latest-and-greatest w32api headers on my system - what do you have? Thanks for finding the off-by-one... blush. Rob
RE: SETUP(PickPackageLine.cc): Patch for 'chopped of characters' problem (RESEND)
-Original Message- From: Ton van Overbeek [mailto:[EMAIL PROTECTED]] Sent: Tuesday, March 26, 2002 9:37 PM To: [EMAIL PROTECTED] Subject: SETUP(PickPackageLine.cc): Patch for 'chopped of characters' problem (RESEND) This time without linewrapping (I hope) Please put the patch in an attached file, rather than inline. Thanks, Rob
Console lseek fix
The following patch allows arch's with-ftp command to run under cygwin.
Chris, any objections?
ChangeLog:
2002-03-15 Robert Collins [EMAIL PROTECTED]
* fhandler.h (fhandler_termios::lseek): Override lseek.
* fhandler_termios.cc (fhandler_termios::lseek): Implement this.
Index: fhandler.h
===
RCS file: /cvs/src/src/winsup/cygwin/fhandler.h,v
retrieving revision 1.110
diff -u -p -r1.110 fhandler.h
--- fhandler.h 2002/02/28 14:25:53 1.110
+++ fhandler.h 2002/03/19 03:13:13
@@ -657,6 +657,7 @@ class fhandler_termios: public fhandler_
void fixup_after_fork (HANDLE);
void fixup_after_exec (HANDLE parent) { fixup_after_fork (parent); }
void echo_erase (int force = 0);
+ virtual __off64_t lseek (__off64_t, int);
};
enum ansi_intensity
Index: fhandler_termios.cc
===
RCS file: /cvs/src/src/winsup/cygwin/fhandler_termios.cc,v
retrieving revision 1.25
diff -u -p -r1.25 fhandler_termios.cc
--- fhandler_termios.cc 2002/03/05 08:15:25 1.25
+++ fhandler_termios.cc 2002/03/19 03:13:13
@@ -345,3 +345,10 @@ fhandler_termios::fixup_after_fork (HAND
this-fhandler_base::fixup_after_fork (parent);
fork_fixup (parent, get_output_handle (), output_handle);
}
+
+__off64_t
+fhandler_termios::lseek (__off64_t, int)
+{
+ set_errno (ESPIPE);
+ return -1;
+}
Re: Patch for cd .../. bug
=== - Original Message - From: Christopher Faylor [EMAIL PROTECTED] To: [EMAIL PROTECTED] Sent: Thursday, March 07, 2002 11:44 AM Subject: Re: Patch for cd .../. bug On Thu, Mar 07, 2002 at 12:39:28AM -, Chris January wrote: This patch fixes the bug that allows cd .../. to succeed. This isn't a bug. It's how Windows works. Windows (NT) has trouble with this too.. see vuln-dev recently. One can convince CMD that it is actually in C:\.. - which is somewhere in the NT device tree IIRC. Anyway, it seems reasonable to me to use unix behaviour for cygwin with this. Rob
IPC test suite
Well, AFAICT I'm now testing every aspect (barring multi-userid support) of sys/ipc.h and sys/shm.h functionality. And the cygwin is passing. The multi-userid support should be present and correct though. So.. for the brave, here tis. Rob ipctest-0.1.3.tar.gz Description: GNU Zip compressed data
RE: Silence pedantic warnings at header file level
Would this also fix the 'is not a prototype' error that cinstall experiences? Rob -Original Message- From: Danny Smith [mailto:[EMAIL PROTECTED]] Sent: Tuesday, March 05, 2002 9:44 AM To: cygwin-patches Subject: RFC: Silence pedantic warnings at header file level GCC 3.x has a a new pragma that causes the rest of the code in the current file to be treated as if it came from a system header Putting this right after the header guard of runtime and w32api headers would silence all the long long and bitfield pedantic warnings that still occur. It would also allow cleanup of the anonymous union __extension__ business. #if defined __GNUC__ __GNUC__ = 3 #pragma GCC system_header #endif This approach is used in GCC's STL headers. Any comments Danny http://movies.yahoo.com.au - Yahoo! Movies - Vote for your nominees in our online Oscars pool.
RE: Daemon patch
Oh, and sorry for the non-separate changelog, I forgot to separate it out. If you'd like that done...let me know. Rob
RE: Daemon patch
Ok, it's done. Minor snafu where I forgot to add the new files :[, but it should all be ok now. Rob -Original Message- From: Christopher Faylor [mailto:[EMAIL PROTECTED]] Sent: Friday, March 01, 2002 12:59 AM To: [EMAIL PROTECTED] Subject: Re: Daemon patch On Thu, Feb 28, 2002 at 08:55:59AM -0500, Christopher Faylor wrote: On Fri, Mar 01, 2002 at 12:44:09AM +1100, Robert Collins wrote: Oh, and sorry for the non-separate changelog, I forgot to separate it out. If you'd like that done...let me know. Nope. Just merge your branch to the head, along with ChangeLog. I reserve the right to do some editing on the ChangeLog, though. :-) You do have all of Corinna's changes in your branch, right? Forgot to mention. Please create a branch tag prior to committing your changes. Something like predaemon, maybe... cgf
Thread.h failure on
Is this patch needed to solve In file included from ../../../../../src/winsup/cygwin/dtable.h:14, from ../../../../../src/winsup/cygwin/cygheap.cc:19: ../../../../../src/winsup/cygwin/thread.h:57: field `_grp' has incomplete type make: *** [cygheap.o] Error 1 or is something else wrong? Index: thread.h === RCS file: /cvs/src/src/winsup/cygwin/thread.h,v retrieving revision 1.33 diff -u -p -r1.33 thread.h --- thread.h2002/02/10 13:50:13 1.33 +++ thread.h2002/02/28 14:37:02 -42,7 +42,7 extern C #include pthread.h #include signal.h #include pwd.h -#include grp.h +#include cygwin/grp.h #define _NOMNTENT_FUNCS #include mntent.h Rob
RE: Thread.h failure on
-Original Message- From: Christopher Faylor [mailto:[EMAIL PROTECTED]] They can be ignored, as the shm and ipc functions aren't exported yet. (Remember: they are incomplete implementations). Well, yeah, but I'd rather not have any warnings in cygwin compilations. I'll fix these up to not warn during the weekend if that's soon enough? Rob
RE: help/version patches
-Original Message-
From: Joshua Daniel Franklin [mailto:[EMAIL PROTECTED]]
It's not - strace is manually entered version info.
Anyway here is a patch for cygcheck that I think does the
right thing. Somebody want to take a look?
+const char *revision=$Revision: 1.22 $;
This is suspect: The revision string can look like $Revision:$ in some circumstances -
see man co again - which is why I had $Revision: $ - note the space after the
second $ sign.
Likewise, the
+ /* Get version number out of the autogenerated revision string */
+ (void *) version = malloc(sizeof(revision));
+ strcpy(version, revision+11);
+ version[strlen(version)-1]= 0;
Could be more robustly done as
+ version = (char *) malloc(sizeof(revision));
+ strcpy(version, revision+11);
+ {
+char *temp=version + strlen (version);
+while (isspace(--temp) temp = version);
+temp[1]='\0';
+ }
Which will munch an arbitrary amount of whitespace.
Rob
RE: [PATCH] Percent complete in Setup.exe window title.
Looks nice. I'll commit to HEAD shortly. Why the discardable stringtable? Rob -Original Message- From: Gary R. Van Sickle [mailto:[EMAIL PROTECTED]] Sent: Sunday, February 24, 2002 6:29 PM To: Cygwin-Patches Subject: [PATCH] Percent complete in Setup.exe window title. This one goes good with the new minimizeability of Setup.exe: 2002-02-24 Gary R. Van Sickle [EMAIL PROTECTED] * res.rc (STRINGTABLE): Add IDS_CYGWIN_SETUP and IDS_CYGWIN_SETUP_WITH_PROGRESS strings. * resource.h: Add IDS_CYGWIN_SETUP and IDS_CYGWIN_SETUP_WITH_PROGRESS IDs. * splash.cc (OnInit): Qualify SetWindowText() call with global scope operator (::SetWindowText()). * threebar.cc: Run indent. (cistring.h): Add include. (SetText1, SetText2, SetText3): Qualify SetWindowText() call with global scope operator. (SetBar2): Add logic for writing percent complete into window title. * window.h: Run indent. (SetWindowText): New function. (String): Add forward declaration. * window.cc: Run indent. (String++.h): Add include. (SetWindowText): New function. -- Gary R. Van Sickle Brewer. Patriot.
Re: [Patch]setup.exe type prefixes for io_stream::mkpath_p and io_stream:open paths
=== - Original Message - From: Michael A Chase [EMAIL PROTECTED] If you are going to use cygfile:// then you shouldn't be calling cygpath(). Whacks self on head. Thanks. Rob
RE: help/version patches
-Original Message- From: Joshua Daniel Franklin [mailto:[EMAIL PROTECTED]] Sent: Monday, February 25, 2002 4:22 PM To: [EMAIL PROTECTED] Subject: help/version patches I've got patches for each of the utils to add/correct the help and version output options. There are 13 in all. I incremented the version number 0.01 from the ones in CVS/Entries with the exception of cygpath. I also added a line based on one found in strace that imbeds the compile date into the version output: Some confusion here: I was meaning that having something like: const char *revision=$Revision: $ ; in the file allows you to then use: const char *version = revision[11]; to obtain the correct version number. Rob
RE: For the curious: Setup.exe char- String patch
-Original Message-
From: Michael A Chase [mailto:[EMAIL PROTECTED]]
I only saw two possible real problems. Everything else is a matter of
consistency which I could send you a patch for after this is
implemented.
String++.cc:
+// does this character exist in the string?
+// 0 is false, 1 is the first position...
+size_t
+String::find(char aChar) const
+{
+ for (size_t i=0; i theData-length; ++i)
+if (theData-theString[i] == aChar)
+ return i;
+ return 0;
### Won't this return 0 if aChar is in the first position in
theData-theString?
Yes. Thank you.
String++.h:
### Do you want String::concat() and String::vconcat to be
public? The few
places I see them being used could be ... String (first) +
next + next ...
You lose a little efficiency by not calling String::concat,
but you make up
some of it by not having to call String::cstr_oneuse().
HMMM, worth thinking about. Remeber that vconcat can only be used with
char *'s, and we don't want them :}. (think unicode native storage).
There are other lower lever mechanisms to optimise String, but as we
aren't CPU bound, I'm not concerned at this point. One such example you
could look at is the SGI Rope class template. (I've not looked at that
but it's similar to what another project I'm on has been creating from
scratch, a team member recently said hey, this is similar :}. As for
concat vs +, concat canonicalises paths, which is what broke Chuck's //
path references (because / indicated a posix path to the code AFAIK). I
don't think thats an appropriate use for String:: though, so wrote +,
and used that. Also, canonicalisation IMO should occur at the
io_stream::open and related calls, not at every manipulation: file path
fragments shouldn't get canonicalised.
archive.cc:
### To be consistent with other log() calls in this file
change the last
line to:
+ {log (LOG_TIMESTAMP, String (Failed to make the path for ) +
destfilename);
...
Yah, it's not 100% complete :]. Let me get it in first, then we can all
chip in an tidy up :}.
geturl.cc:
static void
-init_dialog (char const *url, int length, HWND owner)
+init_dialog (String const url, int length, HWND owner)
start_tics = GetTickCount ();
+ delete[] sl;
}
### Is that delete[] safe? You've been changing sl
repeatedly in the for
loop.
I'll check when I get home. I may have missed it - I've used a temp
variable most other places. Thanks for the catch (assuming it's a bug
:]).
log.cc/log.h:
### If I understand the change, a log line may be either
timestamped or
babble. So a line can't be timestamped and only go to setup.log.full.
Likewise all lines in setup.log must be timestamped. I think
we are losing
some useful capablities by changing from flags to level.
mmm, yes, but we've also gained type safety. If you wish to submit a
flags class ( I can enlarge on that if needed) to allow log
(LOG_BABBLE|LOG_FULL LOG_TIMESTAMP|LOG_LITERAL, String const ) that'd
be fine by me. I like enforcing type safety where possible.
mount.cc:
### It looks like it might be cleaner to make String cygpath
(String const
) visible along with String cygpath (const char *, ...). It
seems like
nearly every place I saw it used you are doing cygpath
(xxx.cstr_oneuse(),0).
Yes, I want to... but doing it was going to be a right ol' pain
initially, so I pu tit to the side.
### The few places that involve concatenation could be handled by
String()+x+... I'm willing to make a patch to catch any
leftovers so String
cygpath( const char *, ...) could be dropped.
Please do.
package_meta.cc:
### I don't think .cstr_oneuse() is needed for log():
+ log (LOG_BABBLE, String(unlink ) + d);
another good catch.
Thanks very much for the detailed analysis.
Rob
RE: For the curious: Setup.exe char- String patch
-Original Message- From: Gary R Van Sickle [mailto:[EMAIL PROTECTED]] I think it would behoove us greatly to duplicate the semantics of std::string here, and return a zero-based offset on success, and an npos on failure. Whats a 'npos' defined as? geturl.cc: static void -init_dialog (char const *url, int length, HWND owner) +init_dialog (String const url, int length, HWND owner) This would be better written const String url. Weeel, yes it would, yet another typo. Fortunately String copies are very lightweight :}. (One addition, one subtraction, and if test and 4 bytes of storage) Rob
Re: For the curious: Setup.exe char- String patch
=== - Original Message - From: Michael A Chase [EMAIL PROTECTED] I'll have to do some thinking on that. I've never tried to create a class. Other changes would be more important at this point. Would a LOG_PLAIN (==LOG_TIMESTAMP) be acceptable to replace 0 in log() calls for now? That way it would be fairly easy to get back to the current log appearance if we decide to. Sure. Rob
Re: small setup.exe fix
=== - Original Message - From: Jan Nieuwenhuizen [EMAIL PROTECTED] To: [EMAIL PROTECTED] Sent: Wednesday, January 30, 2002 8:23 AM Subject: small setup.exe fix Hi, Pressing 'Add' in the `Choose a dowload site' dialogue without typing an url causes a segfault over here. Fix below. Thanks for this, however I've already fixed it in my sandbox, after Chunk reported it to me. Rob
Re: setup.exe command line options
Sorry about the delay getting back to you... === - Original Message - From: [EMAIL PROTECTED] [EMAIL PROTECTED] To: [EMAIL PROTECTED] Sent: Friday, January 25, 2002 12:25 AM Subject: setup.exe command line options I've attached the diffs for the work in progress implementation of command line options. I've tried to copy the dialog's terminology when choosing the options and the variables used within WinMain. Cool. I'm using the PropertyPage Create functions as my interface. In many instances the Create function can do all the work for that dialog, and can return false - so no dialog will be created. I'm having problems with the packagedir and the rootdir Create functions, I must be missing something. Most importantly I'm having difficulty getting yyparse called. :( Simply populate the sites array and the source, and the parsing will happen in the ini dialog automatically. IMO you are better off using the _same_ code - command line parameters != windowless. Much work still to be done, but have a look and let me know how appalled you all are at my strategy! Shock! Horror!. :]. Seriously though, some feedback. * I think that the default window constructors should be left intact - add new constructors,or better yet methods that accept parameters. This will result in cleaner code in main.cc IMO. (i.e. create all the windows, then pass in all the parameters). * (optional) It'd be really neat to have the command line options register themselves, rather than all put in one place. Thats a more OOP approach, but more initial effort required. It also means that the parameter passing methods above wouldn't be needed per se - the registered code would do that on the callback. * for SourcePage, use an enum, not three bools - they are mutually exclusive after all. * Rather than skipping all the pages completely, I'd suggest having the page thread/function post a message that the dialog has finished after your options are processed. This is both simpler (no need to fiddle with what pages exist) and easier (greater ability to leverage the existing code). Other than that, good work. Cheers, Rob
