Bug#609049: [coreutils] Re: Bug#609049: du: process_file: Assertion `level == prev_level - 1' failed

2011-01-10 Thread Jim Meyering 
Jim Meyering wrote:
...
 Here's the incremental I'm testing with:

 diff --git a/tests/du/move-dir-while-traversing 
 b/tests/du/move-dir-while-traversing
 index e42bc93..fe1615c 100755

Thanks again for the quick feedback.
I squashed in the changes above and pushed the result.
I also updated to the latest gnulib in a separate commit,
since it passed the tests I ran.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#609049: [coreutils] Re: Bug#609049: du: process_file: Assertion `level == prev_level - 1' failed

2011-01-09 Thread Jim Meyering 
Jim Meyering wrote:
...
 While I do have a reproducer that will become a test suite addition
 (coming soon), it relies on python (a first) and the python-inotify
 package, so I'll have to be careful to skip the test when those
 prerequisites aren't installed.  Also, there's an inherent race condition,
 so I'll have to find the right compromise between absolute test-robustness
 (too expensive in time and inodes) and reasonableness.

Here's a complete patch, including the test.
I would have preferred to avoid the one-second sleep.
Maybe someone here can see a better way.

From 9bf47055f64efb56d022feca01ad901e85e21bc1 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Sat, 8 Jan 2011 17:44:55 +0100
Subject: [PATCH] du: don't abort when a subdir is renamed during traversal

* NEWS (Bug fixes): Mention it.
* src/du.c (prev_level): Move declaration up to file-scope global.
(du_files): Reset prev_level to 0 upon abnormal fts_read termination.
Reported by Johathan Nieder in http://bugs.debian.org/609049
Also, improve a diagnostic.
* tests/du/move-dir-while-traversing: Test for the above.
* tests/Makefile.am (TESTS): Add it.
---
 NEWS   |8 +++
 src/du.c   |   15 +--
 tests/Makefile.am  |1 +
 tests/du/move-dir-while-traversing |   85 
 4 files changed, 105 insertions(+), 4 deletions(-)
 create mode 100755 tests/du/move-dir-while-traversing

diff --git a/NEWS b/NEWS
index 2a71ca6..5a70243 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,14 @@ GNU coreutils NEWS-*- 
outline -*-

 * Noteworthy changes in release ?.? (-??-??) [?]

+** Bug fixes
+
+  du would abort with a failed assertion when two conditions are met:
+  part of the hierarchy being traversed is moved to a higher level in the
+  directory tree, and there is at least one more command line directory
+  argument following the one containing the moved sub-tree.
+  [bug introduced in coreutils-5.1.0]
+

 * Noteworthy changes in release 8.9 (2011-01-04) [stable]

diff --git a/src/du.c b/src/du.c
index 77deb0c..671cac7 100644
--- a/src/du.c
+++ b/src/du.c
@@ -63,8 +63,11 @@ extern bool fts_debug;
 /* A set of dev/ino pairs.  */
 static struct di_set *di_set;

-/* Define a class for collecting directory information. */
+/* Keep track of the preceding level (depth in hierarchy)
+   from one call of process_file to the next.  */
+static size_t prev_level;

+/* Define a class for collecting directory information. */
 struct duinfo
 {
   /* Size of files in directory.  */
@@ -399,7 +402,6 @@ process_file (FTS *fts, FTSENT *ent)
   struct duinfo dui;
   struct duinfo dui_to_print;
   size_t level;
-  static size_t prev_level;
   static size_t n_alloc;
   /* First element of the structure contains:
  The sum of the st_size values of all entries in the single directory
@@ -582,10 +584,15 @@ du_files (char **files, int bit_flags)
 {
   if (errno != 0)
 {
-  /* FIXME: try to give a better message  */
-  error (0, errno, _(fts_read failed));
+  error (0, errno, _(fts_read failed: %s),
+ quotearg_colon (fts-fts_path));
   ok = false;
 }
+
+  /* When exiting this loop early, be careful to reset the
+ global, prev_level, used in process_file.  Otherwise, its
+ (level == prev_level - 1) assertion could fail.  */
+  prev_level = 0;
   break;
 }
   FTS_CROSS_CHECK (fts);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 06d81f0..a5dbd3e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -370,6 +370,7 @@ TESTS = \
   du/long-from-unreadable  \
   du/long-sloop\
   du/max-depth \
+  du/move-dir-while-traversing \
   du/no-deref  \
   du/no-x  \
   du/one-file-system   \
diff --git a/tests/du/move-dir-while-traversing 
b/tests/du/move-dir-while-traversing
new file mode 100755
index 000..e42bc93
--- /dev/null
+++ b/tests/du/move-dir-while-traversing
@@ -0,0 +1,85 @@
+#!/bin/sh
+# Trigger a failed assertion in coreutils-8.9 and earlier.
+
+# Copyright (C) 2011 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See

Bug#609049: [coreutils] Re: Bug#609049: du: process_file: Assertion `level == prev_level - 1' failed

2011-01-09 Thread Pádraig Brady 
On 09/01/11 22:05, Jim Meyering wrote:
 diff --git a/tests/du/move-dir-while-traversing 
 b/tests/du/move-dir-while-traversing

 +# We use a python-inotify script, so...
 +python -m pyinotify -h  /dev/null \
 +  || skip_ 'python-inotify package not installed'

A small point is that error is a bit fedora specific.
The package is python-pyinotify on debian/ubuntu for example.

 +
 +# Move a directory up while du is processing its sub-directories.
 +# While du is processing a hierarchy .../B/C/D/... this script
 +# detects when du opens D/, and then moves C/ up one level
 +# so that it is a sibling of B/.
 +# Given the inherent race condition, we have to add enough weight
 +# under D/ so that in most cases, the monitor performs the single
 +# rename syscall before du finishes processing the subtree under D/.
 +
 +cat 'EOF'  inotify-watch-for-dir-access.py
 +#!/usr/bin/env python
 +from pyinotify import *

I generally don't include everything from a module,
to keep the namespace clean.
I'd do instead:

import pyinotify as pn
import os,sys
...


 +dir = sys.argv[1]
 +dest_parent = os.path.dirname(os.path.dirname(dir))
 +dest = os.path.join(dest_parent, os.path.basename(dir))
 +
 +class ProcessDir(ProcessEvent):
 +
 +def process_IN_OPEN(self, event):
 +os.rename(dir, dest)
 +sys.exit(0)
 +
 +def process_default(self, event):
 +pass
 +
 +wm = WatchManager()
 +notifier = Notifier(wm)
 +wm.watch_transient_file(dir, IN_OPEN, ProcessDir)
 +print 'started'

The above print is buffered by default.
As we're using it for synchronization I'd

sys.stdout.write('started\n')
sys.stdout.flush()

 +notifier.loop()
 +EOF
 +chmod a+x inotify-watch-for-dir-access.py
 +
 +long=d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z
 +t=T/U
 +mkdir -p $t/3/a/b/c/$long d2 || framework_failure
 +timeout 6 ./inotify-watch-for-dir-access.py $t/3/a/b  start-msg 
 +
 +# Wait for the watcher to start...
 +nonempty() { test -s start-msg  return 0; sleep $1; }
 +retry_delay_ nonempty .1 5

I think the above may iterate only once?
How about:

nonempty() {
  test -s start-msg || { sleep $1; return 1; }
}
retry_delay_ nonempty .1 5 || framework_failure

 +
 +# The above delay is insufficient in ~50% of my trials.
 +# Sometimes, when under heavy load, a parallel make check would
 +# fail this test when sleeping just 0.1 seconds here.
 +sleep 1

Hopefully this extra sleep is not required now

Nice fix BTW!

cheers,
Pádraig.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#609049: [coreutils] Re: Bug#609049: du: process_file: Assertion `level == prev_level - 1' failed

2011-01-09 Thread Jim Meyering 
Pádraig Brady wrote:

 On 09/01/11 22:05, Jim Meyering wrote:
 diff --git a/tests/du/move-dir-while-traversing
 b/tests/du/move-dir-while-traversing

 +# We use a python-inotify script, so...
 +python -m pyinotify -h  /dev/null \
 +  || skip_ 'python-inotify package not installed'

 A small point is that error is a bit fedora specific.
 The package is python-pyinotify on debian/ubuntu for example.

Good point.  I've dropped the -:

  || skip_ 'python inotify package not installed'

 +from pyinotify import *

 I generally don't include everything from a module,
 to keep the namespace clean.
 I'd do instead:

 import pyinotify as pn
 import os,sys
 ...

I prefer that, too.

 +dir = sys.argv[1]
...
 +print 'started'

 The above print is buffered by default.
 As we're using it for synchronization I'd

 sys.stdout.write('started\n')
 sys.stdout.flush()

Good catch!

...
 +nonempty() { test -s start-msg  return 0; sleep $1; }
 +retry_delay_ nonempty .1 5

 I think the above may iterate only once?
 How about:

 nonempty() {
   test -s start-msg || { sleep $1; return 1; }
 }
 retry_delay_ nonempty .1 5 || framework_failure

Yes, that fixes another bug.

 +# The above delay is insufficient in ~50% of my trials.
 +# Sometimes, when under heavy load, a parallel make check would
 +# fail this test when sleeping just 0.1 seconds here.
 +sleep 1

 Hopefully this extra sleep is not required now

Bingo.  It does, so far.
Thanks for the advice and fixes.
Here's the incremental I'm testing with:

diff --git a/tests/du/move-dir-while-traversing 
b/tests/du/move-dir-while-traversing
index e42bc93..fe1615c 100755
--- a/tests/du/move-dir-while-traversing
+++ b/tests/du/move-dir-while-traversing
@@ -21,7 +21,7 @@ print_ver_ du

 # We use a python-inotify script, so...
 python -m pyinotify -h  /dev/null \
-  || skip_ 'python-inotify package not installed'
+  || skip_ 'python inotify package not installed'

 # Move a directory up while du is processing its sub-directories.
 # While du is processing a hierarchy .../B/C/D/... this script
@@ -33,12 +33,14 @@ python -m pyinotify -h  /dev/null \

 cat 'EOF'  inotify-watch-for-dir-access.py
 #!/usr/bin/env python
-from pyinotify import *
+import pyinotify as pn
+import os,sys
+
 dir = sys.argv[1]
 dest_parent = os.path.dirname(os.path.dirname(dir))
 dest = os.path.join(dest_parent, os.path.basename(dir))

-class ProcessDir(ProcessEvent):
+class ProcessDir(pn.ProcessEvent):

 def process_IN_OPEN(self, event):
 os.rename(dir, dest)
@@ -47,10 +49,11 @@ class ProcessDir(ProcessEvent):
 def process_default(self, event):
 pass

-wm = WatchManager()
-notifier = Notifier(wm)
-wm.watch_transient_file(dir, IN_OPEN, ProcessDir)
-print 'started'
+wm = pn.WatchManager()
+notifier = pn.Notifier(wm)
+wm.watch_transient_file(dir, pn.IN_OPEN, ProcessDir)
+sys.stdout.write('started\n')
+sys.stdout.flush()
 notifier.loop()
 EOF
 chmod a+x inotify-watch-for-dir-access.py
@@ -61,14 +64,9 @@ mkdir -p $t/3/a/b/c/$long d2 || framework_failure
 timeout 6 ./inotify-watch-for-dir-access.py $t/3/a/b  start-msg 

 # Wait for the watcher to start...
-nonempty() { test -s start-msg  return 0; sleep $1; }
+nonempty() { test -s start-msg || { sleep $1; return 1; }; }
 retry_delay_ nonempty .1 5

-# The above delay is insufficient in ~50% of my trials.
-# Sometimes, when under heavy load, a parallel make check would
-# fail this test when sleeping just 0.1 seconds here.
-sleep 1
-
 # The above watches for an IN_OPEN event on $t/3/a/b,
 # and when it triggers, moves the parent, $t/3/a, up one level
 # so it's directly under $t.



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org