On Jun 23, 2009, at 8:01 PM, Brock Pytlik wrote:
Updated webrev:
http://cr.opensolaris.org/~bpytlik/ips-9290-v2/
Responses inline below.
Shawn Walker wrote:
line 74: Why not use CachedManifest here?
Um, not sure why I would. This is a tool for developers. I would
suspect that each time the tool was run, the caches would be out of
date since they would have changed their manifest. Further, using
the cachedmanifest would deposit (from their persecptive) random
extra files into the directory they're working in. If/when this gets
integrated into server logic, I'm happy to use a CachedManifest for
that code's entry point.
The main reason to use CachedManifest is a significant memory
savings. I found that when implementing recursive dependency analysis
in pkgrecv, that memory usage could climb to almost a gigabyte if I
checked 'entire', as an example.
I agree about the random files part, which is why I opened a bug to
enhanced CachedManifest to allow you to store the cache files
somewhere else since it doesn't manage them anyway.
Maybe open a bug for this and make it depend on bug 9572?
line 75-77: you need some try/except handling here in case the
open or close fails
It's caught in pkgdep.py. Well, the open is at least. Under what
conditions could closing a file I've opened for reading fail?
file-handle corruption, gone missing, I/O error. It's probably very
rare for reading.
line 78, 79, 81, 84: should these be private properties?
Nope, if they could be protected I'd use that, but subclass do/will/
could need these variables.
Use '_' then to signify their 'protected' status. That's the
convention Danek suggested sometime last year.
pylint also picks up on this and will warn you about violations of it.
modules/publish/dependencies/elf.py:
* The various calls to the elf module in here probably need try/
except wrapping. Some of them already have it.
If you have specific examples, please let me know. This was lifted
straight from solaris.py, so I would suspect the common cases are
being handled correctly.
I wouldn't use solaris.py as an example of all that is good and right ;)
modules/publish/dependencies/elf.py:
line 91: elf.get_info() -- this can raise an exception if there's a
failure trying to read the elf header information (think truncated, I/
O failure, etc.). Conversely, you trap the BadElfError for line 93.
It is possible for get_info() to fail even if is_elf_object succeeds.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
