Re: etc_changed, passwd & group.
On Fri, Jan 17, 2003 at 12:29:05AM -0500, Christopher Faylor wrote: > Maybe not so "less intrusive" after all. I broke out the etc handling > stuff into a separate class and moved even more functionality into > pwdgrp than you did. I hope Corinna approves. As long as it works it's ok with me. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:[EMAIL PROTECTED] Red Hat, Inc.
Re: etc_changed, passwd & group.
> On Thu, Jan 16, 2003 at 02:07:18PM -0500, Christopher Faylor wrote:
> >On Thu, Jan 16, 2003 at 01:57:21AM -0500, Pierre A. Humblet wrote:
> >>Here is the code as it stands. It compiles & runs, and passes
> >>fork tests correctly. Feel free to takeover or at least
> >>have a look. I will continue testing tomorrow evening.
> >>
> >>I include only the 5 files that are related to etc_changed,
> >>the 5 others (setuid on Win9X) can wait.
> >
> >Hmm. I have a slightly less intrusive idea for how to handle this. I'll
> >check it in shortly.
>
> Maybe not so "less intrusive" after all. I broke out the etc handling
> stuff into a separate class and moved even more functionality into
> pwdgrp than you did. I hope Corinna approves.
>
> I also hope that I got all of your changes that didn't conflict with my
> work in. I'm generating a new snapshot now. I guess we should ask
> people to test it for a couple of days before I release 1.3.19. Sigh.
>
> Oh, and I just removed the warning when FindFirstChangeNotification
> fails. This should make the Novell users happy even though the
> performance will be less than wonderful.
>
> Thanks for your patch and your insight, Pierre.
O.K. Chris, I will take a look this weekend. I had moved ALL the etc
handling out of cygheap.cc, into a class in the passwd code, as we had
discussed (I thought).
Also I had merged the pwdgrp_check and pwdgrp_read classes into
a single class because that makes the code much simpler. For example
pwdgrp_check maintains a win32 path but pwdgrp_read recomputes
that very same path each time it is called by pwdgrp_check
(indirectly, through read_etc_passwd). To add a cherry on the cake,
the path kept by pwdgrp_check originally comes from pwdgrp_read.
I had also simplified other parts. For example the file can be closed
and the timestamp updated as soon as the file is read. There is no reason
to wait until the internal passwd structures have been updated.
I agree that the changes look intrusive at first, but from time to
time it helps to streamline the code rather than apply another layer of
fix.
I am also enclosing the little test program I am using. If you run
strace a.exe | fgrep 'Read /etc' you should see what gets updated
where. You have 15 seconds to touch either /etc/passwd or /etc/group.
Running strace sh -c a.exe | fgrep 'Read /etc' should yield a
different behavior because a.exe is execed.
Pierre
#include
#include
main()
{
if (fork() == 0)
{
sleep(16);
printf("Child waking up %x\n", getpwuid(getuid()));
printf("Child waking up %x\n", getgrgid(getgid()));
fflush(stdout);
exit(0);
}
else
{
sleep(15);
printf("Parent waking up %x\n", getgrgid(getgid()));
printf("Parent waking up %x\n", getpwuid(getuid()));
fflush(stdout);
_exit(0);
}
}
Re: etc_changed, passwd & group.
On Fri, Jan 17, 2003 at 10:06:02AM -0500, Pierre A. Humblet wrote: >> On Thu, Jan 16, 2003 at 02:07:18PM -0500, Christopher Faylor wrote: >> >On Thu, Jan 16, 2003 at 01:57:21AM -0500, Pierre A. Humblet wrote: >> >>Here is the code as it stands. It compiles & runs, and passes >> >>fork tests correctly. Feel free to takeover or at least >> >>have a look. I will continue testing tomorrow evening. >> >> >> >>I include only the 5 files that are related to etc_changed, >> >>the 5 others (setuid on Win9X) can wait. >> > >> >Hmm. I have a slightly less intrusive idea for how to handle this. I'll >> >check it in shortly. >> >> Maybe not so "less intrusive" after all. I broke out the etc handling >> stuff into a separate class and moved even more functionality into >> pwdgrp than you did. I hope Corinna approves. >> >> I also hope that I got all of your changes that didn't conflict with my >> work in. I'm generating a new snapshot now. I guess we should ask >> people to test it for a couple of days before I release 1.3.19. Sigh. >> >> Oh, and I just removed the warning when FindFirstChangeNotification >> fails. This should make the Novell users happy even though the >> performance will be less than wonderful. >> >> Thanks for your patch and your insight, Pierre. > >O.K. Chris, I will take a look this weekend. I had moved ALL the etc >handling out of cygheap.cc, into a class in the passwd code, as we had >discussed (I thought). >Also I had merged the pwdgrp_check and pwdgrp_read classes into >a single class because that makes the code much simpler. For example >pwdgrp_check maintains a win32 path but pwdgrp_read recomputes >that very same path each time it is called by pwdgrp_check >(indirectly, through read_etc_passwd). To add a cherry on the cake, >the path kept by pwdgrp_check originally comes from pwdgrp_read. >I had also simplified other parts. For example the file can be closed >and the timestamp updated as soon as the file is read. There is no reason >to wait until the internal passwd structures have been updated. This is what I did, too. I only took a glance at what you'd done and thought it could be done a little more simply. As it turns out, I was wrong, but the end result was that I thoroughly understood the code again, so I checked in what I'd done + any of your changes that added more functionality. >I am also enclosing the little test program I am using. If you run >strace a.exe | fgrep 'Read /etc' you should see what gets updated >where. You have 15 seconds to touch either /etc/passwd or /etc/group. >Running strace sh -c a.exe | fgrep 'Read /etc' should yield a >different behavior because a.exe is execed. I wrote a similar program and watched the results as I modified /etc/passwd. I also added some strace output to ensure that /etc/passwd wasn't just being read repeatedly. It didn't seem like it was. So, I think the end result should be the same even though the implementation differs slightly. I basically kept most of Corinna's infrastructure. I also merged the two pwdgrp* classes, introduced a new etc class used by pwdgrp, got rid of close and get_fname, made gets private, renamed 'open' to 'load' since it is responsible for loading the whole passwd/group, and introduced the concept of an /etc "handle" which can be registered with a specific filename. I'm not entirely happy with the handle implementation but I thought that it might be useful if we wanted to do something similar with other files in /etc someday. My "intrusive" changes are the same as yours, though. All of the *_state variables got renamed in my patch, same as yours. That's why I said 'not so "less intrusive" after all'. cgf
nanosleep() patch
Attached is a patch that implements nanosleep() by attempting to
reuse the current sleep() implementation which seems to provide the
necessary functionality.
I'm not sure if there is a better way to convey the fact that
sleep_worker() was interrupted than my current implementation.
Comments on this issue and the patch in general are welcome.
Thanks,
Jason
--
PGP/GPG Key: http://www.tishler.net/jason/pubkey.asc or key servers
Fingerprint: 7A73 1405 7F2B E669 C19D 8784 1AFD E4CC ECF4 8EF6
Index: cygwin.din
===
RCS file: /cvs/src/src/winsup/cygwin/cygwin.din,v
retrieving revision 1.74
diff -u -p -r1.74 cygwin.din
--- cygwin.din 17 Jan 2003 13:08:05 - 1.74
+++ cygwin.din 17 Jan 2003 18:43:15 -
@@ -597,6 +597,8 @@ nan
_nan = nan
nanf
_nanf = nanf
+nanosleep
+_nanosleep = nanosleep
nextafter
_nextafter = nextafter
nextafterf
Index: signal.cc
===
RCS file: /cvs/src/src/winsup/cygwin/signal.cc,v
retrieving revision 1.41
diff -u -p -r1.41 signal.cc
--- signal.cc 15 Jan 2003 10:21:23 - 1.41
+++ signal.cc 17 Jan 2003 18:43:15 -
@@ -66,37 +66,47 @@ signal (int sig, _sig_func_ptr func)
return prev;
}
-extern "C" unsigned int
-sleep (unsigned int seconds)
+static bool
+sleep_worker (DWORD req, DWORD& rem)
{
int rc;
sig_dispatch_pending (0);
sigframe thisframe (mainthread);
- DWORD ms, start_time, end_time;
+ DWORD start_time, end_time;
+ bool res = false;
pthread_testcancel ();
- ms = seconds * 1000;
start_time = GetTickCount ();
- end_time = start_time + (seconds * 1000);
- syscall_printf ("sleep (%d)", seconds);
+ end_time = start_time + req;
+ syscall_printf ("sleep_worker (%ld)", req);
- rc = pthread::cancelable_wait (signal_arrived, ms);
+ rc = pthread::cancelable_wait (signal_arrived, req);
DWORD now = GetTickCount ();
if (rc == WAIT_TIMEOUT || now >= end_time)
-ms = 0;
+rem = 0;
else
-ms = end_time - now;
+rem = end_time - now;
if (WaitForSingleObject (signal_arrived, 0) == WAIT_OBJECT_0)
-(void) thisframe.call_signal_handler ();
-
- DWORD res = (ms + 500) / 1000;
- syscall_printf ("%d = sleep (%d)", res, seconds);
+{
+ (void) thisframe.call_signal_handler ();
+ res = true;
+}
+ syscall_printf ("%d = sleep_worker (%ld, %ld)", res, req, rem);
return res;
}
extern "C" unsigned int
+sleep (unsigned int seconds)
+{
+ DWORD req = seconds * 1000;
+ DWORD rem = 0;
+ sleep_worker (req, rem);
+ return (rem + 500) / 1000;
+}
+
+extern "C" unsigned int
usleep (unsigned int useconds)
{
pthread_testcancel ();
@@ -105,6 +115,34 @@ usleep (unsigned int useconds)
syscall_printf ("usleep (%d)", useconds);
pthread::cancelable_wait (signal_arrived, (useconds + 500) / 1000);
syscall_printf ("0 = usleep (%d)", useconds);
+ return 0;
+}
+
+extern "C" int
+nanosleep (const struct timespec *rqtp, struct timespec *rmtp)
+{
+ if (rqtp->tv_sec < 0 || rqtp->tv_nsec < 0 || rqtp->tv_nsec > 9)
+{
+ set_errno (EINVAL);
+ return -1;
+}
+
+ DWORD req = rqtp->tv_sec * 1000 + (rqtp->tv_nsec + 50) / 100;
+ DWORD rem = 0;
+ bool signal = sleep_worker (req, rem);
+
+ if (rmtp)
+{
+ rmtp->tv_sec = rem / 1000;
+ rmtp->tv_nsec = (rem % 1000) * 100;
+}
+
+ if (signal)
+{
+ set_errno (EINTR);
+ return -1;
+}
+
return 0;
}
Index: include/cygwin/version.h
===
RCS file: /cvs/src/src/winsup/cygwin/include/cygwin/version.h,v
retrieving revision 1.96
diff -u -p -r1.96 version.h
--- include/cygwin/version.h17 Jan 2003 13:08:06 - 1.96
+++ include/cygwin/version.h17 Jan 2003 18:43:15 -
@@ -169,12 +169,13 @@ details. */
69: Export strtof
70: Export asprintf, _asprintf_r, vasprintf, _vasprintf_r
71: Export strerror_r
+ 72: Export nanosleep
*/
/* Note that we forgot to bump the api for ualarm, strtoll, strtoull */
#define CYGWIN_VERSION_API_MAJOR 0
-#define CYGWIN_VERSION_API_MINOR 71
+#define CYGWIN_VERSION_API_MINOR 72
/* There is also a compatibity version number associated with the
shared memory regions. It is incremented when incompatible
2003-01-17 Jason Tishler <[EMAIL PROTECTED]>
* cygwin.din: Export nanosleep().
* signal.cc (sleep_worker): New function.
(sleep): Move essential old functionality to sleep_worker(). Call
sleep_worker().
(nanosleep): New function.
* include/cygwin/version.h: Bump DLL minor number.
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 <[EMAIL PROTECTED]>
* 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.
Index: pwdgrp.h
===
RCS file: /cvs/src/src/winsup/cygwin/pwdgrp.h,v
retrieving revision 1.11
diff -u -p -r1.11 pwdgrp.h
--- pwdgrp.h17 Jan 2003 18:05:32 - 1.11
+++ pwdgrp.h18 Jan 2003 04:16:15 -
@@ -32,12 +32,9 @@ class etc
{
static int curr_ix;
static bool sawchange[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 bool file_changed (int);
- static void set_last_modified (int, FILETIME&);
+ static int init (int, bool);
+ static bool file_changed (int, const char *, FILETIME *);
friend class pwdgrp;
};
@@ -46,6 +43,7 @@ class pwdgrp
pwdgrp_state state;
int pwd_ix;
path_conv pc;
+ FILETIME last_modified;
char *buf;
char *lptr, *eptr;
@@ -71,38 +69,35 @@ class pwdgrp
public:
bool isinitializing ()
-{
- if (state <= initializing)
- state = initializing;
- else if (etc::file_changed (pwd_ix - 1))
- state = initializing;
- return state == initializing;
-}
+ {
+if (state <= initializing)
+ state = initializing;
+else if (etc::file_changed (pwd_ix -
Re: etc_changed, passwd & group
Chris, An ugly tought came to my mind while doing the dishes: there is a race condition left. FindFirstChangeNotification must be called from init, otherwise in the normal case it will be called after the load, leaving a window where the file can be updated without being noticed. Similarly in the fork case, where changed_dir is called first, FindFirstChangeNotification must be called from changed_dir (if the handle is NULL), even though res will be true. I guess this calls for defining a new function en etc::, calling it both from init and changed_dir. Enough for today, good night. Pierre
