This is the mail archive of the cygwin-developers@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]

Re: [RFA] A kinder, gentler check for /etc/{passwd,group} changes


On Sat, Sep 08, 2001 at 10:51:33PM -0400, Christopher Faylor wrote:
> Here's what I did, based on the FindFirstFileChangeNotification ideas.
> 
> It seems to get performance back down to around 1.3.2 levels when
> combined with a couple of other minor changes.
> 
> Now that I see the patch, I realize that the etc_changed function
> probably belongs in miscfuncs.cc rather than in dcrt0.cc.

IMO it belongs into the class init_cygheap.  I did it that way in
my version of that patch.

> The only thing I don't know is if the etc_changed function actually
> does anything useful.  I don't have a useful test case for that but
> I thought that Corinna might.

It works.  A simple test is ssh.  Start sshd, login using your
account.  Edit /etc/passwd to use tcsh as login shell instead of
bash.  Try another login.

Testing the /etc/group stuff isn't that easy.  The current Cygwin
version doesn't support supplementary groups to add when the user
context changes due to a NTCreateToken() call.  I plan to add it
to 1.3.4 though.

> I wonder if we could generalize the similar code in passwd.cc and grp.cc
> into some kind of class for 1.3.4...

See my below patch. The whole class stuff is now in a common
header file `pwdgrp.h'.  There could be more common stuff later,
perhaps.

On Sun, Sep 09, 2001 at  1:14:27AM -0400, Christopher Faylor wrote:
> >Huh? We _have_ to read the file once. I don't think that extracting the
> >modification date from an openfile is a likely performance hit!.
> 
> Hmm.  I was referring to the way that Corinna did this originally.  She
> used FindFirstFile to get the date.  I'm not sure why she didn't use   
> GetFileInformationByHandle, or something, but you're right, this
> wouldn't be a huge performance hit.

I did that since opening a file and getting the modification date
by GetFileInformationByHandle() is more costly than FindFirstFile().

Think of the difference.  Getting the modification time when opened
the file the first time would be cheap since the file handle is
already open.  Getting the modification time on each file check
by opening the file and calling GetFileInf...() isn't that cheap
anymore.

Adding a check for the modification time in the new etc_changed()
stuff should be no problem since it's now not called on each
getpwXXX()/getgrXXX() call but only when _additionally_ some /etc
member has changed.  I have added some small additional tweaks to
passwd.cc and grp.cc which result in some more performance optimization
in my test case, building bash-2.05:

Before Chris patch:	97.5s
After Chris patch:	85.8s
Now:			79.2s

I have attached the patch relativ to the current CVS version.  So it
includes your code, too, Chris, just moved from dcrt0.cc to cygheap.cc.

If it's ok I will check it in today.

Corinna

Index: cygheap.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/cygheap.cc,v
retrieving revision 1.38
diff -u -p -r1.38 cygheap.cc
--- cygheap.cc	2001/09/06 18:06:27	1.38
+++ cygheap.cc	2001/09/09 13:20:36
@@ -16,6 +16,7 @@
 #include "security.h"
 #include "fhandler.h"
 #include "dtable.h"
+#include "path.h"
 #include "cygheap.h"
 #include "child_info.h"
 #include "heap.h"
@@ -343,6 +344,39 @@ cstrdup1 (const char *s)
   strcpy (p, s);
   MALLOC_CHECK;
   return p;
+}
+
+bool
+init_cygheap::etc_changed ()
+{
+  bool res = 0;
+
+  if (!etc_changed_h)
+    {
+      path_conv pwd ("/etc");
+      etc_changed_h = FindFirstChangeNotification (pwd, FALSE,
+					      FILE_NOTIFY_CHANGE_LAST_WRITE);
+      if (etc_changed_h == INVALID_HANDLE_VALUE)
+	system_printf ("Can't open /etc for checking, %E", (char *) pwd,
+	    	       etc_changed_h);
+      else if (!DuplicateHandle (hMainProc, etc_changed_h, hMainProc,
+	    			 &etc_changed_h, 0, TRUE,
+				 DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE))
+	{
+	  system_printf ("Can't inherit /etc handle, %E", (char *) pwd,
+	      		 etc_changed_h);
+	  etc_changed_h = INVALID_HANDLE_VALUE;
+	}
+    }
+
+   if (etc_changed_h != INVALID_HANDLE_VALUE
+       && WaitForSingleObject (etc_changed_h, 0) == WAIT_OBJECT_0)
+     {
+       (void) FindNextChangeNotification (etc_changed_h);
+       res = 1;
+     }
+
+  return res;
 }
 
 void
Index: cygheap.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/cygheap.h,v
retrieving revision 1.22
diff -u -p -r1.22 cygheap.h
--- cygheap.h	2001/09/07 21:32:04	1.22
+++ cygheap.h	2001/09/09 13:20:36
@@ -161,8 +161,11 @@ struct init_cygheap
   mode_t umask;
   HANDLE shared_h;
   HANDLE console_h;
+  HANDLE etc_changed_h;
   cwdstuff cwd;
   dtable fdtab;
+
+  bool etc_changed ();
 };
 
 #define CYGHEAPSIZE (sizeof (init_cygheap) + (4000 * sizeof (fhandler_union)) + (2 * 65536))
Index: grp.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/grp.cc,v
retrieving revision 1.29
diff -u -p -r1.29 grp.cc
--- grp.cc	2001/09/07 21:32:04	1.29
+++ grp.cc	2001/09/09 13:20:36
@@ -26,6 +26,7 @@ details. */
 #include "path.h"
 #include "cygheap.h"
 #include "cygerrno.h"
+#include "pwdgrp.h"
 
 /* Read /etc/group only once for better performance.  This is done
    on the first call that needs information from it. */
@@ -42,58 +43,8 @@ static int max_lines;
 static int grp_pos = 0;
 #endif
 
-/* Set to loaded when /etc/passwd has been read in by read_etc_passwd ().
-   Set to emulated if passwd is emulated. */
-/* Functions in this file need to check the value of passwd_state
-   and read in the password file if it isn't set. */
-enum grp_state {
-  uninitialized = 0,
-  initializing,
-  emulated,
-  loaded
-};
-class grp_check {
-  grp_state state;
-  FILETIME  last_modified;
-  char	    grp_w32[MAX_PATH];
+static pwdgrp_check group_state;
 
-public:
-  grp_check () : state (uninitialized)
-    {
-      last_modified.dwLowDateTime = last_modified.dwHighDateTime = 0;
-      grp_w32[0] = '\0';
-    }
-  operator grp_state ()
-    {
-      HANDLE h;
-      WIN32_FIND_DATA data;
-
-      if (!grp_w32[0])	/* First call. */
-	{
-	  path_conv g ("/etc/group", PC_SYM_FOLLOW | PC_FULL);
-	  if (!g.error)
-	    strcpy (grp_w32, g.get_win32 ());
-	}
-
-      if ((h = FindFirstFile (grp_w32, &data)) != INVALID_HANDLE_VALUE)
-	{
-	  if (CompareFileTime (&data.ftLastWriteTime, &last_modified) > 0)
-	    {
-	      state = uninitialized;
-	      last_modified = data.ftLastWriteTime;
-	    }
-	  FindClose (h);
-	}
-      return state;
-    }
-  void operator = (grp_state nstate)
-    {
-      state = nstate;
-    }
-};
-
-static grp_check group_state;
-
 static int
 parse_grp (struct group &grp, const char *line)
 {
@@ -215,6 +166,7 @@ read_etc_group ()
 		add_grp_line (linebuf);
 	    }
 
+	  group_state.set_last_modified (f);
 	  fclose (f);
 	  group_state = loaded;
 	}
Index: passwd.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/passwd.cc,v
retrieving revision 1.31
diff -u -p -r1.31 passwd.cc
--- passwd.cc	2001/09/07 21:32:04	1.31
+++ passwd.cc	2001/09/09 13:20:37
@@ -1,6 +1,6 @@
 /* passwd.cc: getpwnam () and friends
 
-   Copyright 1996, 1997, 1998 Cygnus Solutions.
+   Copyright 1996, 1997, 1998, 2001 Cygnus Solutions.
 
 This file is part of Cygwin.
 
@@ -23,6 +23,7 @@ details. */
 #include "pinfo.h"
 #include "cygheap.h"
 #include <sys/termios.h>
+#include "pwdgrp.h"
 
 /* Read /etc/passwd only once for better performance.  This is done
    on the first call that needs information from it. */
@@ -31,58 +32,8 @@ static struct passwd *passwd_buf;	/* pas
 static int curr_lines;
 static int max_lines;
 
-/* Set to loaded when /etc/passwd has been read in by read_etc_passwd ().
-   Set to emulated if passwd is emulated. */
-/* Functions in this file need to check the value of passwd_state
-   and read in the password file if it isn't set. */
-enum pwd_state {
-  uninitialized = 0,
-  initializing,
-  emulated,
-  loaded
-};
-class pwd_check {
-  pwd_state state;
-  FILETIME  last_modified;
-  char	    pwd_w32[MAX_PATH];
+static pwdgrp_check passwd_state;
 
-public:
-  pwd_check () : state (uninitialized)
-    {
-      last_modified.dwLowDateTime = last_modified.dwHighDateTime = 0;
-      pwd_w32[0] = '\0';
-    }
-  operator pwd_state ()
-    {
-      HANDLE h;
-      WIN32_FIND_DATA data;
-
-      if (!pwd_w32[0])	/* First call. */
-	{
-	  path_conv p ("/etc/passwd", PC_SYM_FOLLOW | PC_FULL);
-	  if (!p.error)
-	    strcpy (pwd_w32, p.get_win32 ());
-	}
-
-      if ((h = FindFirstFile (pwd_w32, &data)) != INVALID_HANDLE_VALUE)
-	{
-	  if (CompareFileTime (&data.ftLastWriteTime, &last_modified) > 0)
-	    {
-	      state = uninitialized;
-	      last_modified = data.ftLastWriteTime;
-	    }
-	  FindClose (h);
-	}
-      return state;
-    }
-  void operator = (pwd_state nstate)
-    {
-      state = nstate;
-    }
-};
-
-static pwd_check passwd_state;
-
 
 /* Position in the passwd cache */
 #ifdef _MT_SAFE
@@ -200,6 +151,7 @@ read_etc_passwd ()
 		  add_pwd_line (linebuf);
 	      }
 
+	    passwd_state.set_last_modified (f);
 	    fclose (f);
 	    passwd_state = loaded;
 	  }
Index: pwdgrp.h
===================================================================
diff -u -p pwdgrp.h.nil pwdgrp.h
--- pwdgrp.h.nil	Sun Sep  9 15:21:40 2001
+++ pwdgrp.h	Sun Sep  9 15:17:09 2001
@@ -0,0 +1,57 @@
+/* pwdgrp.h
+
+   Copyright 2001 Red Hat inc.
+
+   Stuff common to pwd and grp handling.
+
+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. */
+
+enum pwdgrp_state {
+  uninitialized = 0,
+  initializing,
+  emulated,
+  loaded
+};
+
+class pwdgrp_check {
+  pwdgrp_state	state;
+  FILETIME	last_modified;
+  char		file_w32[MAX_PATH];
+
+public:
+  pwdgrp_check () : state (uninitialized) {}
+  operator pwdgrp_state ()
+    {
+      if (state != uninitialized && file_w32[0] && cygheap->etc_changed ())
+        {
+          HANDLE h;
+          WIN32_FIND_DATA data;
+
+          if ((h = FindFirstFile (file_w32, &data)) != INVALID_HANDLE_VALUE)
+            {
+              if (CompareFileTime (&data.ftLastWriteTime, &last_modified) > 0)
+                state = uninitialized;
+              FindClose (h);
+            }
+        }
+      return state;
+    }
+  void operator = (pwdgrp_state nstate)
+    {
+      state = nstate;
+    }
+  void set_last_modified (FILE *f)
+    {
+      if (!file_w32[0])
+        strcpy (file_w32, cygheap->fdtab[fileno (f)]->get_win32_name ());
+
+      BY_HANDLE_FILE_INFORMATION inf;
+      if (GetFileInformationByHandle (cygheap->fdtab[fileno (f)]->get_handle (),
+                                      &inf))
+        last_modified = inf.ftLastWriteTime;
+    }
+};

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Developer                                mailto:cygwin@cygwin.com
Red Hat, Inc.


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