[nfs-discuss] Code review request: Connectathon NFS tests improvements
> Hi Gordon, Hi Rob, Thanks for having a look at this stuff. > I am mostly happy with your changes; I think they're a > useful addition to a test suite that gets intermittent > love :-) > > One thing - the special tests used to copy themselves to > the target and run from there, and you have changed it to > at least not always do that. I understand why we need a > way to avoid executing from the target remote filesystem, > but executing that way has at least a time or two shown > off bugs, mostly in the client, because of the way exec() > manages mappings of the binary. I would prefer that this > continue to be the default mode of operation to maintain > this coverage, and to have an option to have the special > tests do a chdir() instead. This is not huge, but I'd > rather not lose the regression coverage. > > Rob T I could make that change to the "special" tests optional, but I'm not sure that would really be an improvement. The changes to the "special" tests to run them from where they are instead of copying into the mount point was for two reasons: (1) allow running them on an FS with "nomap", and (2) make the tests cover just the specific features each was designed to exercise. It appeared to me that covering exec/mmap in those tests was an accident of implementation. Further, if there are failures in what was being tested by the "special" tests, the practice of running the tests in the mount would only serve to complicate the debugging efforts because much of the access via the mount would have nothing to do with what the tests are really trying to cover. One could interpret the change in the "special" tests as a reduction of coverage (because it no longer covers exec) but the test suite as a whole does appear to cover the mmap/exec cases reasonably well in the "general" tests. What I'd prefer to do with this is to augment the "general" tests (if necessary) to cover whatever we think may be no longer adequately covered after the changes in "special". Would you be OK with that plan? Can you think of any specific exec/mmap cases not covered by the current "general" tests? (i.e. the test failure you mentioned you had seen previously?) Thanks, Gordon
[nfs-discuss] Code review request: Connectathon NFS tests improvements
Gordon Ross wrote: > What I'd prefer to do with this is to augment the "general" > tests (if necessary) to cover whatever we think may be no > longer adequately covered after the changes in "special". > > Would you be OK with that plan? In general, sure, though I can't point to something specific to add. Running some code from the target filesystem (with that as a specific intent) would seem like a good thing. Rob T
[nfs-discuss] Code review request: Connectathon NFS tests improvements
Gordon Ross wrote: > Now the more interesting stuff: > > Some time ago, I observed that the Connectathon NFS tests > would often terminate early after any test error. Hi Gordon, I am mostly happy with your changes; I think they're a useful addition to a test suite that gets intermittent love :-) One thing - the special tests used to copy themselves to the target and run from there, and you have changed it to at least not always do that. I understand why we need a way to avoid executing from the target remote filesystem, but executing that way has at least a time or two shown off bugs, mostly in the client, because of the way exec() manages mappings of the binary. I would prefer that this continue to be the default mode of operation to maintain this coverage, and to have an option to have the special tests do a chdir() instead. This is not huge, but I'd rather not lose the regression coverage. Rob T
[nfs-discuss] Code review request: Connectathon NFS tests improvements
Thanks for having a look. Yes, I'll be happy to take your changes. (If I'm not careful, I may end up owning this "pile":) > Hi Gordon, > > I gave it a quick sanity test on Mac OS X 10.6. It looks good. > Although it looks like you left some debug stuff in one of the > scripts: Yes, I'll remove those. > # debug > echo "TESTS=$TESTS" > echo "TESTARG=$TESTARG" > echo "TESTPATH=$TESTPATH" > echo "passes=$passes" > > > Also your post reminded me that I have a couple minor MacOSX-related > changes to the suite waiting to to be submitted. Would you be > interested in accepting those? Or should I find someone else to > funnel them through? > > Thanks! > --macko
[nfs-discuss] Code review request: Connectathon NFS tests improvements
Please make sure the diffs are applied against the gate: [thud at ultralord ~/src]> hg clone ssh://anon at hg.opensolaris.org//hg/nfsv41/cthon destination directory: cthon requesting all changes adding changesets adding manifests adding file changes added 4 changesets with 157 changes to 157 files updating working directory 157 files updated, 0 files merged, 0 files removed, 0 files unresolved For now, we can do code reviews on this alias...
[nfs-discuss] Code review request: Connectathon NFS tests improvements
And here are the attachments. [doh!]
On Fri, Feb 12, 2010 at 06:55:22PM -0800, Mike Mackovitch wrote:
> On Fri, Feb 12, 2010 at 08:36:41PM -0500, Gordon Ross wrote:
> > Thanks for having a look. Yes, I'll be happy to take your changes.
> > (If I'm not careful, I may end up owning this "pile":)
>
> Cool, I've attached a small patch... it's mostly minor stuff.
> (Actually two small patches: one against the gate and one against
> your "fixwarn" version. One of my changes hits an ifdef right next
> to one of your changes.)
>
> The patch does also include one change to lock test #12 that I needed
> when I was testing running the Mac OS X NFSv4 client against a server
> that was constantly rebooting. Test #12 has the child take a lock and
> then the parent kills the child and checks that the lock has been
> released after a couple seconds. The problem I hit was that if the
> server goes down before the child's lock can be released, then the
> parent will mistakenly report that the test failed when it's really
> just waiting on the server to come back up. To get around that I added
> some calls to the parent to open/close/unlink a separate file before
> it checks that the child's lock has been released. These operations
> should hold up the parent long enough to allow the child's lock to be
> released even if the server was down at the time.
>
> Feedback is welcome from any NFS experts. :-)
>
> Thanks!
> --macko
> ___
> nfs-discuss mailing list
> nfs-discuss at opensolaris.org
-- next part --
diff -r ba2cc26c2e5f lock/tlock.c
--- a/lock/tlock.c Tue Feb 24 11:53:55 2009 -0800
+++ b/lock/tlock.c Fri Feb 12 18:40:14 2010 -0800
@@ -1390,6 +1390,7 @@
{
if (who == PARENT) {
pid_t target;
+ char testfile2[256];
close(pidpipe[1]);
@@ -1408,6 +1409,12 @@
kill(target, SIGINT);
comment("Killed child process.");
sleep(wait_time);
+ comment("Opening/closing/unlinking another file.");
+ sprintf(&testfile2[0], "%s/otherfile%d", filepath, parentpid);
+ close(open(testfile2, O_RDWR|O_CREAT, 0666));
+ unlink(testfile2);
+ comment("Waiting a little more.");
+ sleep(wait_time);
test(12, 1, F_TLOCK, (off_t)0, (off_t)0, PASS, FATAL);
childfree(0);
close_testfile(DO_UNLINK);
diff -r ba2cc26c2e5f tests.init
--- a/tests.initTue Feb 24 11:53:55 2009 -0800
+++ b/tests.initFri Feb 12 18:40:14 2010 -0800
@@ -40,6 +40,7 @@
# SVR4
# Solaris 2.x
# HPUX
+# Mac OS X 10.5+
DASHN=
BLC=\\c
@@ -48,7 +49,7 @@
# SunOS 4.X
# Linux
# Tru64 UNIX
-# Mac OS X
+# Mac OS X <10.4
#DASHN=-n
#BLC=
@@ -214,9 +215,9 @@
#CC=gcc
# Use with Mac OS X
-#CFLAGS=`echo -DMACOSX -DNATIVE64 -DLARGE_LOCKS`
+#CFLAGS=`echo -DMACOSX -DMMAP -DSTDARG -DNATIVE64 -DLARGE_LOCKS
-DHAVE_SOCKLEN_T`
#MOUNT=/sbin/mount
#UMOUNT=/sbin/umount
-#LOCKTESTS=`echo tlock`
+#LOCKTESTS=`echo tlocklfs`
# ---
diff -r ba2cc26c2e5f tools/dirdmp.c
--- a/tools/dirdmp.cTue Feb 24 11:53:55 2009 -0800
+++ b/tools/dirdmp.cFri Feb 12 18:40:14 2010 -0800
@@ -35,7 +35,7 @@
int argc;
char *argv[];
{
-#if defined(LINUX) || defined (AIX)
+#if defined(LINUX) || defined (AIX) || defined(MACOSX)
fprintf(stderr, "dirdmp is not supported on this platform.\n");
exit(1);
#else
@@ -44,10 +44,10 @@
while (argc--) {
print(*argv++);
}
-#endif /* LINUX || AIX */
+#endif /* LINUX || AIX || MACOSX */
}
-#if !(defined(LINUX) || defined(AIX))
+#if !(defined(LINUX) || defined(AIX) || defined(MACOSX))
static void
print(dir)
@@ -217,4 +217,4 @@
}
}
-#endif /* LINUX || AIX */
+#endif /* LINUX || AIX || MACOSX */
-- next part --
diff -ur cthon.orig/lock/tlock.c cthon.new/lock/tlock.c
--- cthon.orig/lock/tlock.c 2010-02-10 20:25:59.0 -0800
+++ cthon.new/lock/tlock.c 2010-02-12 14:50:35.0 -0800
@@ -1390,6 +1390,7 @@
{
if (who == PARENT) {
pid_t target;
+ char testfile2[256];
close(pidpipe[1]);
@@ -1408,6 +1409,12 @@
kill(target, SIGINT);
comment("Killed child process.");
sleep(wait_time);
+ comment("Opening/closing/unlinking another file.");
+ sprintf(&testfile2[0], "%s/otherfile%d", filepath, parentpid);
+ close(open(testfile2, O_RDWR|O_CREAT, 0666));
+ unlink(testfile2);
+ comment("Waiting a little more.");
+ sleep(wait_time);
test(12, 1, F_TLOCK, (off_t)0, (off_t)0, PASS, FATAL);
childfree(0);
close_testfile(
[nfs-discuss] Code review request: Connectathon NFS tests improvements
On Fri, Feb 12, 2010 at 08:36:41PM -0500, Gordon Ross wrote: > Thanks for having a look. Yes, I'll be happy to take your changes. > (If I'm not careful, I may end up owning this "pile":) Cool, I've attached a small patch... it's mostly minor stuff. (Actually two small patches: one against the gate and one against your "fixwarn" version. One of my changes hits an ifdef right next to one of your changes.) The patch does also include one change to lock test #12 that I needed when I was testing running the Mac OS X NFSv4 client against a server that was constantly rebooting. Test #12 has the child take a lock and then the parent kills the child and checks that the lock has been released after a couple seconds. The problem I hit was that if the server goes down before the child's lock can be released, then the parent will mistakenly report that the test failed when it's really just waiting on the server to come back up. To get around that I added some calls to the parent to open/close/unlink a separate file before it checks that the child's lock has been released. These operations should hold up the parent long enough to allow the child's lock to be released even if the server was down at the time. Feedback is welcome from any NFS experts. :-) Thanks! --macko
[nfs-discuss] Code review request: Connectathon NFS tests improvements
On Thu, Feb 11, 2010 at 08:54:01AM -0800, Gordon Ross wrote: > Two change sets - first the "boring" part: Fix compiler warnings, etc. > > http://cr.opensolaris.org/~gwr/cthon-nfs-tests-fixwarn/webrev/ > [...] >http://cr.opensolaris.org/~gwr/cthon-nfs-tests-keepgoing/webrev/ > [...] > > So far I've tried this version on: Solaris NFS, Solaris smbfs, > and a Linux (Ubuntu 9.4) cifs mount. > > Feedback appreciated. Hi Gordon, I gave it a quick sanity test on Mac OS X 10.6. It looks good. Although it looks like you left some debug stuff in one of the scripts: # debug echo "TESTS=$TESTS" echo "TESTARG=$TESTARG" echo "TESTPATH=$TESTPATH" echo "passes=$passes" Also your post reminded me that I have a couple minor MacOSX-related changes to the suite waiting to to be submitted. Would you be interested in accepting those? Or should I find someone else to funnel them through? Thanks! --macko
[nfs-discuss] Code review request: Connectathon NFS tests improvements
Two change sets - first the "boring" part: Fix compiler warnings, etc. http://cr.opensolaris.org/~gwr/cthon-nfs-tests-fixwarn/webrev/ Now the more interesting stuff: Some time ago, I observed that the Connectathon NFS tests would often terminate early after any test error. That may be helpful for regression testing, but in most cases it will give better coverage if the tests try to keep running, and just report whatever failures happened. These changes add a "-k" flag (for "keep going") to the various runtests scripts, which when set, makes the tests keep going where possible. Test scripts now show a summary of failures at completion, so failures are obvious even when using the -k flag. There are also minor changes to some of the tests to make them more tolerant of unsupported functionality, also with the goal of letting tests continue as much as possible. (This allows using the tests on smbfs mounts, etc.) Here is the "keepgoing" change set: http://cr.opensolaris.org/~gwr/cthon-nfs-tests-keepgoing/webrev/ In both cases, I uploaded the whole workspace, so if you want to try these out, you can just "wget -r" from the URL above with the /webrev/ part removed. Then try: cd ...keepgoing make ./runtests -f -k -a /test/mount/location So far I've tried this version on: Solaris NFS, Solaris smbfs, and a Linux (Ubuntu 9.4) cifs mount. Feedback appreciated. Thanks, Gordon -- This message posted from opensolaris.org
