Shawn Walker wrote:
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.
Well, first remember we're only going to be holding one of these in
memory at a time (per process). Second, if the cached info isn't there,
the first thing CachedManifest does is create a normal manifest, so
there won't be any memory savings if we nuke the cached files each time.
Third, and most importantly from my perspective, there's nothing right
now that assures that the cache is as new or newer than the manifest
being be read. That's not usually a problem on the client or the server
where we assume manifests are fixed; however, the model here assumes the
developer as actively developing the manifest, that's why they're
running the program.
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?
I think using a Manifest is the right decision for the near future. If
you want to file a bug, go ahead but I'm not inclined to.
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.
Since we don't have catches around closes things anyplace else in the
code, I think it's reasonable to forgo cluttering up the code here too.
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.
Huh, I missed that convention, or had forgotten about it. Ok, I'll do
that then in the next round of changes.
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 ;)
Well, since it's been running over the wos without blowing up, I'm
inclined to treat is as reasonably well tested for the time being. That
said, I'll catch the exception you point out below unless I come across
a reason not to.
Brock
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,
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
