On Wed, Nov 12, 2008 at 09:18:35PM -0800, Brock Pytlik wrote:

>>   - line 1451: wow, this is a bit dense.  There are some things in here
>>     that don't make complete sense to me, but I'll try to rephrase it
>>     anyway.
>>
>>         try:
>>             mod_time = os.stat(cache_file).st_mtime
>>         except EnvironmentError, e:
>>             if e.errno == errno.ENOENT:
>>                 mod_time = None
>>             else:
>>                 raise
>>
>>         if not self._catalog or self._cache_reread or \
>>           mod_time != self._catalog_cache_mod_time:
>>             try:
>>                 load_pkl()
>>             except blah:
>>                 raise Runtime
>>
>>             # This should come after the attempt to load the pickle;
>>             # otherwise the modtime isn't always going to be a good test.
>>             self._catalog_cache_mod_time = mod_time
>
> I think you captured the basic intent. One thing is I'm pretty sure about 
> though is that self._catalog_cache_mod_time should be set before the file 
> is read, otherwise in (extremely perverse and adversarial situations) you 
> could up with a self._ccmt of a file newer than had been read into memory. 

So the way I have it above, the modtime is read prior to reading the file,
which means that it could be out of date by the time the file is read,
leading to a second reading -- erroring on the side of safety.

Caching that modtime, though, if done before we read the file and fail,
means that the time we cache is potentially later than the copy we have in
memory, meaning we won't try to reread the file next time through, even
though we need to.

To be ultra-paranoid, though, you could set self._catalog = {} in the
except clause (and even _ccmt to None there, too), essentially forcing it
to be retried next time through.

Here's one error, though, w.r.t. the out-of-catalog packages -- if we error
out in load_catalog_cache(), that information never gets loaded.  Something
to think about for later.

>> Are the test changes related to 4886?
>>   
> Yes, the idea is to change the catalog.pkl file out from under the existing 
> api object and make sure the info is accurately updated. I realized after 
> you asked about them, that a better test would be to create a separate api 
> object to do the installation. I'm running with these changes now.

Okay.  Please be sure to comment what you're doing.

Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Reply via email to