This is the mail archive of the cygwin-patches@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: etc_changed, passwd & group


Hello Chris,

I like your code, just made a few changes. The explanations below
are detailed and longer that the changes!

In load(), CloseFile wasn't called on read error, and it was called
before GetFileTime. 

It seems that declaring passwd_buf, group_buf etc NO_COPY means that
there will be a memory leak on fork, as the malloced buffers they point to
will be orphans. Thus I made them regular static variables and I reuse
the existing buffers on fork (same logic as it was before). Thus the pwdgrp
class cannot be NO_COPY either, nor etc::curr_ix. 
Reusing the buffers creates a problem because the file(s) may have been
touched
after the buffers were loaded in the parent, but before the fork.
dir_changed in the 
child will not observe the event. Thus on forks it is necessary to check
the file
dates when the buffers are already loaded, but not if they are not. 
This is achieved by having the default sawchange as forcing a date
comparison and
passing an argument to etc::init to set sawchange if the compare is not
needed.
The meaning of sawchange is changed: sawchange[n] is false if a change has
not been 
seen by n (seems logical).

sawchange[n] is now set in dir_changed itself, and not in file_changed
nor in set_last_modified(). Setting it in set_last_modified becomes
unnecessary
due to the way sawchange is initialized (also that created a tiny race
condition, I think). That made set_last_modified a one line function. 
I decided to get rid of it and to store last_modified in the pwdgrp
class itself, where it is easier to update it. By the same token I removed 
fn from etc, simply passing the filename in file_changed. 
 
In passwd.cc and grp.cc I reverted to the debug_printf giving the line count,
it has served me well in the past. The return value of load was then unused,
so I made it void, which simplified load a little.

This passes my test...

Pierre

P.S.: I have mixed feelings about having dir_changed always returning true if 
  FindFirstChangeNotification has failed. The etc_changed stuff has been
broken
  until now and this has generated only one complaint AFAIK. Users seem
  quite willing to restart their daemons after editing passwd. If only Novell
  users (with /etc on a share drive) need to do it now, that will already be 
  a big improvement.


2003/01/17  Pierre Humblet  <pierre.humblet@ieee.org>

	* pwdgrp.h (class etc): Remove members fn, last_modified and 
	set_last_modified. Change arguments of init and file changed.
	(class pwdgrp): Add member last_modified.
	(pwdgrp::isinitializing): White space. Add arguments to file_changed.
	(pwdgrp::load): Change type to void. Change second argument of etc::init.
	Close file on read error, but never before calling GetFileTime.
	Do not call etc::set_last_modified.
	* uinfo.cc: Remove definitions of etc::fn and etc::last_modified. Do not
	define etc::curr_ix NO_COPY.
	(etc::init): Use second argument to initialize sawchange. Do not initialize
	fn.
	(etc:::dir_changed): sawchange[n] is now false when a change has happened
	but has not been seen by n. Initialize res accordingly. On observing a 
	change set the sawchange array to false. Always set sawchange[n] to true.
	(etc::file_changed): Use new arguments. Do not set sawchange.
	Do not verify fn, FindFirstFile accepts NULL pointers.
	(etc::set_last_modified): Delete.
	* passwd.cc: Do not define passwd_buf, curr_lines, maxlines and pr NO_COPY.
	(read_etc_passwd): Revert to old style debug_printf. 
	* group.cc: Do not define group_buf, curr_lines, maxlines and gr NO_COPY.
	(read_etc_group): Revert to old style debug_printf. 

Attachment: pwgr.diff
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]