Re: [Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-12-07 Thread Simon Schampijer 

On 12/07/2010 12:13 AM, James Cameron wrote:

On Mon, Dec 06, 2010 at 10:05:43AM +0100, Simon Schampijer wrote:

Small nitpick from me: use os.path.join instead of str methods to
concatenate paths.

self._pending_files.append(os.path.join(dir_path, entry))


Yes, I saw that too, but the existing code uses

for entry in dirs:
if entry.startswith('.'):
continue
full_path = dir_path + '/' + entry

and trying to change code unnecessarily is how defects are introduced,
as has already been seen.

I also don't like the idea of doing changes for portability unless they
can be tested on alternate platforms.

The same pattern occurs in extensions/cpsection/language/model.py,
src/jarabe/model/network.py, src/jarabe/model/shell.py, and
src/sugar/activity/activityservice.py if you'd like to change it.
Activities InfoSlicer, Read, and TamTam are also afflicted.


Ok, patches welcome :)


For the patch title I would prefer something like "Make scanning of
storage devices more robust OLPC #10140", is slightly more
descriptive.


Right.


Acked-By: Simon Schampijer


Thanks.



Great, can you please push it? /me wants to do a new 0.84 release.

Regards,
   Simon
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

Re: [Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-12-06 Thread James Cameron 
On Mon, Dec 06, 2010 at 10:05:43AM +0100, Simon Schampijer wrote:
> Small nitpick from me: use os.path.join instead of str methods to
> concatenate paths.
> 
> self._pending_files.append(os.path.join(dir_path, entry))

Yes, I saw that too, but the existing code uses

for entry in dirs:
if entry.startswith('.'):
continue
full_path = dir_path + '/' + entry

and trying to change code unnecessarily is how defects are introduced,
as has already been seen.

I also don't like the idea of doing changes for portability unless they
can be tested on alternate platforms.

The same pattern occurs in extensions/cpsection/language/model.py,
src/jarabe/model/network.py, src/jarabe/model/shell.py, and
src/sugar/activity/activityservice.py if you'd like to change it.
Activities InfoSlicer, Read, and TamTam are also afflicted.

> For the patch title I would prefer something like "Make scanning of
> storage devices more robust OLPC #10140", is slightly more
> descriptive.

Right.

> Acked-By: Simon Schampijer 

Thanks.

-- 
James Cameron
http://quozl.linux.org.au/
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

Re: [Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-12-06 Thread Simon Schampijer 

On 11/30/2010 03:32 PM, Sascha Silbe wrote:

Excerpts from James Cameron's message of Tue Nov 30 01:14:14 +0100 2010:

[src/jarabe/journal/model.py]


+if self._date_start is not None and self.st_mtime<  self._date_start:
+return
+
+if self._date_end is not None and self.st_mtime>  self._date_end:
+return


Both occurrences of self.st_mtime need to read stat.st_mtime instead
(found by pylint).

With those changes:

Tested-By: Sascha Silbe
Reviewed-By: Sascha Silbe

Sascha


Yes, please do those changes noted from Sascha. Small nitpick from me: 
use os.path.join instead of str methods to concatenate paths.


self._pending_files.append(os.path.join(dir_path, entry))

For the patch title I would prefer something like "Make scanning of 
storage devices more robust OLPC #10140", is slightly more descriptive.


Regards,
   Simon

Acked-By: Simon Schampijer 
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

Re: [Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-12-01 Thread James Cameron 
On Tue, Nov 30, 2010 at 03:32:57PM +0100, Sascha Silbe wrote:
> Excerpts from James Cameron's message of Tue Nov 30 01:14:14 +0100 2010:
> 
> [src/jarabe/journal/model.py]
> 
> > +if self._date_start is not None and self.st_mtime < 
> > self._date_start:
> > +return
> > +
> > +if self._date_end is not None and self.st_mtime > self._date_end:
> > +return
> 
> Both occurrences of self.st_mtime need to read stat.st_mtime instead
> (found by pylint).

Sigh.  You suggested splitting these tests, expanding the scope of my
patch.  I now regret accepting the suggestion.  I think I should stick
to the changes I want to make, not the changes other people want me to
make.  When enthusiasm for a change is low, the defect rate is high.

-- 
James Cameron
http://quozl.linux.org.au/
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

Re: [Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-11-30 Thread Sascha Silbe 
Excerpts from James Cameron's message of Tue Nov 30 01:14:14 +0100 2010:

[src/jarabe/journal/model.py]

> +if self._date_start is not None and self.st_mtime < self._date_start:
> +return
> +
> +if self._date_end is not None and self.st_mtime > self._date_end:
> +return

Both occurrences of self.st_mtime need to read stat.st_mtime instead
(found by pylint).

With those changes:

Tested-By: Sascha Silbe 
Reviewed-By: Sascha Silbe 

Sascha

--
http://sascha.silbe.org/
http://www.infra-silbe.de/


signature.asc
Description: PGP signature
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

Re: [Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-11-29 Thread James Cameron 
This patch was proposed on 14th September and there was no response.

-- 
James Cameron
http://quozl.linux.org.au/
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

[Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-11-29 Thread James Cameron 
Update the progress bar regularly and prevent the UI from stalling
during a scan.  Avoid following recursive symlinks, and symlinks that
point outside the filesystem being scanned.  Do not check for MIME type
if file is excluded for other filter reasons.  Do not report permission
denied errors.
---
 src/jarabe/journal/model.py |  151 +++---
 1 files changed, 97 insertions(+), 54 deletions(-)

diff --git a/src/jarabe/journal/model.py b/src/jarabe/journal/model.py
index 0773446..a8836e3 100644
--- a/src/jarabe/journal/model.py
+++ b/src/jarabe/journal/model.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2007-2008, One Laptop Per Child
+# Copyright (C) 2007-2010, One Laptop per Child
 #
 # 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
@@ -16,10 +16,11 @@
 
 import logging
 import os
+import errno
 from datetime import datetime
 import time
 import shutil
-from stat import S_IFMT, S_IFDIR, S_IFREG
+from stat import S_IFLNK, S_IFMT, S_IFDIR, S_IFREG
 import re
 from operator import itemgetter
 
@@ -230,7 +231,9 @@ class InplaceResultSet(BaseResultSet):
 BaseResultSet.__init__(self, query, page_size)
 self._mount_point = mount_point
 self._file_list = None
-self._pending_directories = 0
+self._pending_directories = []
+self._visited_directories = []
+self._pending_files = []
 self._stopped = False
 
 query_text = query.get('query', '')
@@ -257,7 +260,10 @@ class InplaceResultSet(BaseResultSet):
 
 def setup(self):
 self._file_list = []
-self._recurse_dir(self._mount_point)
+self._pending_directories = [self._mount_point]
+self._visited_directories = []
+self._pending_files = []
+gobject.idle_add(self._scan)
 
 def stop(self):
 self._stopped = True
@@ -298,63 +304,100 @@ class InplaceResultSet(BaseResultSet):
 
 return entries, total_count
 
-def _recurse_dir(self, dir_path):
-self._pending_directories += 1
-gobject.idle_add(self._idle_recurse_dir, dir_path)
-
-def _idle_recurse_dir(self, dir_path):
-try:
-self._real_recurse_dir(dir_path)
-finally:
-self._pending_directories -= 1
-if self._pending_directories == 0:
-self.setup_ready()
-
-def _real_recurse_dir(self, dir_path):
+def _scan(self):
 if self._stopped:
+return False
+
+self.progress.send(self)
+
+if self._pending_files:
+self._scan_a_file()
+return True
+
+if self._pending_directories:
+self._scan_a_directory()
+return True
+
+self.setup_ready()
+self._visited_directories = []
+return False
+
+def _scan_a_file(self):
+full_path = self._pending_files.pop(0)
+
+try:
+stat = os.lstat(full_path)
+except OSError, e:
+if e.errno != errno.ENOENT:
+logging.exception(
+'Error reading metadata of file %r', full_path)
+return
+
+if S_IFMT(stat.st_mode) == S_IFLNK:
+try:
+link = os.readlink(full_path)
+except OSError, e:
+logging.exception(
+'Error reading target of link %r', full_path)
+return
+
+if not os.path.abspath(link).startswith(self._mount_point):
+return
+
+try:
+stat = os.stat(full_path)
+
+except OSError, e:
+if e.errno != errno.ENOENT:
+logging.exception(
+'Error reading metadata of linked file %r', full_path)
+return
+
+if S_IFMT(stat.st_mode) == S_IFDIR:
+id_tuple = stat.st_ino, stat.st_dev
+if not id_tuple in self._visited_directories:
+self._visited_directories.append(id_tuple)
+self._pending_directories.append(full_path)
 return
 
+if S_IFMT(stat.st_mode) != S_IFREG:
+return
+
+if self._regex is not None and \
+not self._regex.match(full_path):
+return
+
+if self._date_start is not None and self.st_mtime < self._date_start:
+return
+
+if self._date_end is not None and self.st_mtime > self._date_end:
+return
+
+if self._mime_types:
+mime_type = gio.content_type_guess(filename=full_path)
+if mime_type not in self._mime_types:
+return
+
+file_info = (full_path, stat, int(stat.st_mtime), stat.st_size)
+self._file_list.append(file_info)
+
+return
+
+def _scan_a_directory(self):
+dir_path = self._pending_directories.pop(0)
+
 try:
-dirs = os.listdir(dir_path)
-except

[Sugar-devel] [PATCH] fix journal scan of external media, dev.laptop.org #10140

2010-09-13 Thread James Cameron 
Update the progress bar regularly and prevent the UI from stalling
during a scan.  Avoid following recursive symlinks, and symlinks that
point outside the filesystem being scanned.  Do not check for MIME type
if file is excluded for other filter reasons.  Do not report permission
denied errors.

Tested on Sugar 0.88 using Ubuntu Sugar Remix (maverick).
---
 src/jarabe/journal/model.py |  150 ---
 1 files changed, 97 insertions(+), 53 deletions(-)

diff --git a/src/jarabe/journal/model.py b/src/jarabe/journal/model.py
index 81ca7d4..5ba3a76 100644
--- a/src/jarabe/journal/model.py
+++ b/src/jarabe/journal/model.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2007-2008, One Laptop Per Child
+# Copyright (C) 2007-2010, One Laptop per Child
 #
 # 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
@@ -16,10 +16,11 @@
 
 import logging
 import os
+import errno
 from datetime import datetime
 import time
 import shutil
-from stat import S_IFMT, S_IFDIR, S_IFREG
+from stat import S_IFLNK, S_IFMT, S_IFDIR, S_IFREG
 import re
 from operator import itemgetter
 
@@ -220,7 +221,9 @@ class InplaceResultSet(BaseResultSet):
 BaseResultSet.__init__(self, query, page_size)
 self._mount_point = mount_point
 self._file_list = None
-self._pending_directories = 0
+self._pending_directories = []
+self._visited_directories = []
+self._pending_files = []
 self._stopped = False
 
 query_text = query.get('query', '')
@@ -247,7 +250,10 @@ class InplaceResultSet(BaseResultSet):
 
 def setup(self):
 self._file_list = []
-self._recurse_dir(self._mount_point)
+self._pending_directories = [self._mount_point]
+self._visited_directories = []
+self._pending_files = []
+gobject.idle_add(self._scan)
 
 def stop(self):
 self._stopped = True
@@ -287,62 +293,100 @@ class InplaceResultSet(BaseResultSet):
 
 return entries, total_count
 
-def _recurse_dir(self, dir_path):
-self._pending_directories += 1
-gobject.idle_add(self._idle_recurse_dir, dir_path)
-
-def _idle_recurse_dir(self, dir_path):
-try:
-self._real_recurse_dir(dir_path)
-finally:
-self._pending_directories -= 1
-if self._pending_directories == 0:
-self.setup_ready()
-
-def _real_recurse_dir(self, dir_path):
+def _scan(self):
 if self._stopped:
+return False
+
+self.progress.send(self)
+
+if self._pending_files:
+self._scan_a_file()
+return True
+
+if self._pending_directories:
+self._scan_a_directory()
+return True
+
+self.setup_ready()
+self._visited_directories = []
+return False
+
+def _scan_a_file(self):
+full_path = self._pending_files.pop(0)
+
+try:
+stat = os.lstat(full_path)
+except OSError, e:
+if e.errno != errno.ENOENT:
+logging.exception(
+'Error reading metadata of file %r', full_path)
+return
+
+if S_IFMT(stat.st_mode) == S_IFLNK:
+try:
+link = os.readlink(full_path)
+except OSError, e:
+logging.exception(
+'Error reading target of link %r', full_path)
+return
+
+if not os.path.abspath(link).startswith(self._mount_point):
+return
+
+try:
+stat = os.stat(full_path)
+
+except OSError, e:
+if e.errno != errno.ENOENT:
+logging.exception(
+'Error reading metadata of linked file %r', full_path)
+return
+
+if S_IFMT(stat.st_mode) == S_IFDIR:
+id_tuple = stat.st_ino, stat.st_dev
+if not id_tuple in self._visited_directories:
+self._visited_directories.append(id_tuple)
+self._pending_directories.append(full_path)
 return
 
+if S_IFMT(stat.st_mode) != S_IFREG:
+return
+
+if self._regex is not None and \
+not self._regex.match(full_path):
+return
+
+if self._date_start is not None and self.st_mtime < self._date_start:
+return
+
+if self._date_end is not None and self.st_mtime > self._date_end:
+return
+
+if self._mime_types:
+mime_type = gio.content_type_guess(filename=full_path)
+if mime_type not in self._mime_types:
+return
+
+file_info = (full_path, stat, int(stat.st_mtime), stat.st_size)
+self._file_list.append(file_info)
+
+return
+
+def _scan_a_directory(self):
+dir_path = self._pending_directories.pop(0)
+
 t