Mostly good news recently: 1) Chris' 51 line ChangeLog on 01-19 is his personal best since Sept 2000. When combined with the 31 line ChangeLog on 01-16, they exceed by far his record for the millennium (67 lines in Sept 2000). This is in a thread that has "Hmm. I have a slightly less intrusive idea for how to handle this. I'll check it in shortly." Sorry Chris, I can't stop chuckling. It relieves the frustration you mentioned, but no hard feelings.
2) The code contains nice features and seems to work, although I find it needlessly confusing and don't fully understand it. I attach a few changes (mostly deletions and reversions) that IMO simplify it significantly while keeping Chris's code partitioning: - "change_possible" is back to boolean instead of ternary values. - arrays are back to size MAX_ETC_FILES - no duplicated calls to test_file_change on Novell - other useless etc::file_changed and etc::init calls avoided. 3) For future reference, the way the modified code works can be explained relatively simply, to the point where I feel I could write a formal proof. a) Initially the pwdgrp::state is "uninitialized". This causes all internal and external get{pw,gr}XX functions to call read_etc_{passwd,group} and thus pwdgrp::load(filename). b) When pwdgrp::load is called in the uninitialized state, it calls etc::init, which allocates an "ix handle", records a pointer to the w32 path, and calls etc::file_changed, thus indirectly etc::dir_changed (with "change_possible" initialized to false). If the /etc handle is NULL, FindFirstFileNotification is called and the "change_possible" array is set to true. If the handle is not NULL, change_possible is already true. The current filetime is recorded and the "change_possible" entry of the file itself is set to false. load() then loads the file and changes the state to "loaded". c) Subsequent calls to get{pw,gr}XX invoke pwdgrp::isinitializing, which calls etc::file_changed if the state is "loaded". etc::file_changed first calls etc::dir_changed, which returns true if change_possible is true or if /etc is on Novell or if /etc has changed. In this last case the array change_possible is set to true to force time stamp checks on all files. If dir_changed returns true, the file time is checked and file_changed returns true, always resetting change_possible for the file to false. If file_changed returns true, isinitializing () changes the state to "initializing" (insuring that it returns the same value when called several times in a row, e.g. due to mutex checks). read_etc_yy and pwdgrp::load are called, loading the file and setting the state to "loaded" (without calling etc::init). Possible changes in the w32 path are recorded by load and will be taken into account in future calls to etc::file_changed. d) Following a fork in the loaded state, the first call (in the child) to dir_changed (from 3) with false change_possible will detect that the NO_COPY /etc handle has been reset to NULL. As in (2) it calls FindFirstFileNotification and sets the change_possible array to true, which will eventually trigger a check of the timestamps. Note that: -any change to /etc/ or a fork will trigger a timestamp check. -a timestamp check is only performed: a) Initially (to set the time) b) Following a change to /etc c) After a fork d) On Novell. 4) To finish off the work and leave the code in a newly minted state, I suggest the following cleanup (mostly deletions): - class pwdgrp Delete operator = - pwdgrp::load Delete "fh = NULL" and use a common CloseHandle, instead of duplicating it. Instead of having type bool just to produce an "it failed" message, use type void and put a debug_printf on lines that have "res = false". - etc::file_changed Remove the test (!fn[n]) because a) it is never true (pc will always return a pointer to its path) and b) even if it was true, NULL is legal in FindFirstFile. If you insist on keeping it, move the test to init(). - etc::test_file_changed Consider reverting the code back to etc::file_changed. It is only called from that very short function. - etc::change_possible For modularity move "change_possible[n] = 0;" from file_changed to dir_changed. Move definition of "change_possible" from etc class to a static in dir_changed, exactly as changed_h already is. Alternatively put the handle and the string "/etc" in the etc class, to make it applicable to any directory. - etc::last_modified Remove from etc class and declare static inside test_file_changed, see change_possible above. 5) The not so good news: apparently dir_changed() is not triggered by a mv or an rm (WinME). cp and file edits do trigger it. 2003/01/20 Pierre Humblet <[EMAIL PROTECTED]> * pwdgrp.h (pwdgrp::isinitializing): Do not change the state when it is uninitialized. * uinfo.cc (pwdfrp::load): Only call etc::init when the state is uninitialized. * path.h: (class etc): Revert the type of change_possible to bool and the array sizes to MAX_ETC_FILES. * path.cc: (etc::change_possible): Revert type to bool and size to MAX_ETC_FILES. (etc::fn): Revert size to MAX_ETC_FILES. (etc::last_modified): Ditto. (etc::init): Delete first argument. Return curr_ix as handle. Call file_changed() instead of test_file_changed(). (etc::test_file_change): Do not test for negative values of change_possible and do not set it to -res. (etc::dir_changed): When the handle is NULL, call memset instead of test_file_changed. When the handle is invalid, return true.
Index: pwdgrp.h =================================================================== RCS file: /cvs/src/src/winsup/cygwin/pwdgrp.h,v retrieving revision 1.12 diff -u -p -r1.12 pwdgrp.h --- pwdgrp.h 20 Jan 2003 02:57:54 -0000 1.12 +++ pwdgrp.h 21 Jan 2003 00:09:57 -0000 @@ -53,11 +53,10 @@ public: void add_line (char *); bool isinitializing () { - if (state <= initializing) + if (state == loaded + && etc::file_changed (pwd_ix)) state = initializing; - else if (etc::file_changed (pwd_ix)) - state = initializing; - return state == initializing; + return state <= initializing; } void operator = (pwdgrp_state nstate) { state = nstate; } bool isuninitialized () const { return state == uninitialized; } Index: uinfo.cc =================================================================== RCS file: /cvs/src/src/winsup/cygwin/uinfo.cc,v retrieving revision 1.101 diff -u -p -r1.101 uinfo.cc --- uinfo.cc 20 Jan 2003 02:57:54 -0000 1.101 +++ uinfo.cc 21 Jan 2003 00:10:08 -0000 @@ -429,7 +429,8 @@ pwdgrp::load (const char *posix_fname) buf = NULL; pc.check (posix_fname); - pwd_ix = etc::init (pwd_ix, pc); + if (state == uninitialized) + pwd_ix = etc::init (pc); paranoid_printf ("%s", posix_fname); Index: path.h =================================================================== RCS file: /cvs/src/src/winsup/cygwin/path.h,v retrieving revision 1.49 diff -u -p -r1.49 path.h --- path.h 20 Jan 2003 02:57:54 -0000 1.49 +++ path.h 21 Jan 2003 00:10:16 -0000 @@ -213,13 +213,12 @@ int path_prefix_p (const char *path1, co class etc { static int curr_ix; - static signed char change_possible[MAX_ETC_FILES + 1]; - static const char *fn[MAX_ETC_FILES + 1]; - static FILETIME last_modified[MAX_ETC_FILES + 1]; + static bool change_possible[MAX_ETC_FILES]; + static const char *fn[MAX_ETC_FILES]; + static FILETIME last_modified[MAX_ETC_FILES]; static bool dir_changed (int); - static int init (int, const char *); + static int init (const char *); static bool file_changed (int); - static void set_last_modified (int, FILETIME&); static bool test_file_change (int); friend class pwdgrp; }; Index: path.cc =================================================================== RCS file: /cvs/src/src/winsup/cygwin/path.cc,v retrieving revision 1.240 diff -u -p -r1.240 path.cc --- path.cc 20 Jan 2003 02:57:54 -0000 1.240 +++ path.cc 21 Jan 2003 00:10:31 -0000 @@ -3756,25 +3756,23 @@ out: int etc::curr_ix = 0; /* Note that the first elements of the below arrays are unused */ -signed char etc::change_possible[MAX_ETC_FILES + 1]; -const char *etc::fn[MAX_ETC_FILES + 1]; -FILETIME etc::last_modified[MAX_ETC_FILES + 1]; +bool etc::change_possible[MAX_ETC_FILES]; +const char *etc::fn[MAX_ETC_FILES]; +FILETIME etc::last_modified[MAX_ETC_FILES]; int -etc::init (int n, const char *etc_fn) +etc::init (const char *etc_fn) { - if (n > 0) - /* ok */; - else if (++curr_ix <= MAX_ETC_FILES) - n = curr_ix; + if (curr_ix < MAX_ETC_FILES) + { + fn[curr_ix] = etc_fn; + (void) file_changed (curr_ix); + } else api_fatal ("internal error"); - fn[n] = etc_fn; - change_possible[n] = false; - (void) test_file_change (n); - paranoid_printf ("fn[%d] %s, curr_ix %d", n, fn[n], curr_ix); - return n; + paranoid_printf ("fn[%d] %s, curr_ix %d", curr_ix, fn[curr_ix], curr_ix); + return curr_ix++; } bool @@ -3784,12 +3782,7 @@ etc::test_file_change (int n) WIN32_FIND_DATA data; bool res; - if (change_possible[n] < 0) - { - res = true; - paranoid_printf ("fn[%d] %s, already marked changed", n, fn[n]); - } - else if ((h = FindFirstFile (fn[n], &data)) == INVALID_HANDLE_VALUE) + if ((h = FindFirstFile (fn[n], &data)) == INVALID_HANDLE_VALUE) { res = true; memset (last_modified + n, 0, sizeof (last_modified[n])); @@ -3800,7 +3793,6 @@ etc::test_file_change (int n) FindClose (h); res = CompareFileTime (&data.ftLastWriteTime, last_modified + n) > 0; last_modified[n] = data.ftLastWriteTime; - change_possible[n] = -res; debug_printf ("FindFirstFile succeeded"); } @@ -3825,12 +3817,11 @@ etc::dir_changed (int n) system_printf ("Can't open /etc for checking, %E", (char *) pwd, changed_h); #endif - for (int i = 1; i <= curr_ix; i++) - (void) test_file_change (i); + memset (change_possible, true, sizeof change_possible); } if (changed_h == INVALID_HANDLE_VALUE) - (void) test_file_change (n); /* semi-brute-force way */ + return true; else if (WaitForSingleObject (changed_h, 0) == WAIT_OBJECT_0) { (void) FindNextChangeNotification (changed_h);