[Sugar-devel] [PATCH sugar-0.84] journal scan of external media

[Sascha Silbe]

Excerpts from James Cameron's message of Thu Sep 09 03:24:36 +0200 2010:

> > > +        if None not in [self._date_start, self._date_end] and \
> > > +            (stat.st_mtime < self._date_start or
> > > +             stat.st_mtime > self._date_end):
> > > +            add_to_list = False
> > 
> > How about either checking just self._date_start or splitting it up into
> > two separate rules?
> 
> Don't understand, sorry.

I mean either:

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

Or:

        if self._date_start is not None and not
                (self._date_start <= self.st_mtime <= self._date_end):
            return True

> > We should use a single way to derive MIME types (sugar.mime) in our code
> > to avoid using different MIME types in different parts of Sugar.
> I don't understand sugar.mime enough to do that.

OK, then we'll tackle that in a separate patch. FYI: This is the code used
e.g. in jarabe.journal.palettes.ObjectPalette.__friend_select_cb():

        mime_type = mime.get_for_file(file_name)

(with mime = sugar.mime)


[ENOTDIR during os.listdir()]
> The reproducible instance is /media/disk/dev/fd/24 ... since fd is a
> symlink to /proc/self/fd ... (awesome isn't it?  One can insert a USB key
> that will list the contents of any directory on the laptop, provided
> access is allowed.)

Ah, it's good that came up during testing, didn't think about it. We
should make sure symlinks don't point outside the filesystem by resolving
them (os.path.abspath()) and checking against the root path.

> On nine test filesystems of varying sizes, the following exceptions were
> counted:
[...]
Thanks for the details statistics!

> os.lstat of an entry returned by os.listdir
[...]
>     exception EIO a total of 7 times

Interesting. Was the storage medium broken in some way (read error)?
I don't even see EIO as a documented error code for lstat(2).

> On the test filesystem of 73k files, the pending directory list in raw
> form is about 250k, and worst case whole path list from "find /mnt" is
> 4MB.

OK, that sounds reasonable enough even given Pythons list overhead.
FWIW, the output of "find / -xdev" on my XO-1.5 is 12.5MB (installed TeX
recently).

> > Why? The order in which os.listdir() returns the entries is undefined
> > anyway.
> 
> When one uses os.listdir and then reverses the order before going
> through them, the resulting performance is low, since the files are
> being accessed in reverse.  Because I didn't know about pop(0), I had to
> use .reverse() to maintain ordering.

Ah, don't you love how undefined behaviour still affects performance?


[_scan()]
> +    if len(self._pending_files) > 0:
> +        return self._scan_a_file()
> +
> +    if len(self._pending_directories) > 0:
> +        return self._scan_a_directory()

PEP-8 says we should use "if self._pending_*" instead (without len()).

If we just call the functions and return True in _scan(), we don't
need to ensure that we always return True in self._scan_a_*().


[_scan_a_directory()]
> +    try:
> +        entries = os.listdir(dir_path)
> +    except OSError, e:
> +        if e.errno not in [errno.EACCES, errno.ENOTDIR]:
> +        logging.exception('Error reading directory %r', dir_path)
> +        return True

Once we fix the symlink issues, ENOTDIR shouldn't happen anymore.

Sascha

--
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
Url : http://lists.sugarlabs.org/archive/sugar-devel/attachments/20100910/b0162dfb/attachment-0001.pgp