Re: etc_changed, passwd & group

["Pierre A. Humblet" <Pierre dot Humblet at ieee dot org>]

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  <pierre.humblet@ieee.org>

	* 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.

Attachment: try.diff
Description: Text document