Thanks for the comments. Please see inline. I've updated the webrev based on the changes:
http://cr.opensolaris.org/~tmueller/cr-954-2/ The code in os_windows.py is substantially restructured, so please have a look again. Danek Duvall wrote: > On Thu, Jun 19, 2008 at 02:06:54PM -0500, Tom Mueller (pkg-discuss) wrote: > > >> http://cr.opensolaris.org/~tmueller/cr-954/ >> > > os_windows.py: > > - I'm confused why rename() and cleanup_trash() don't share the same > cache variable for the image, or the same codepath to discover it. > Good point. I've refactored the code to make these use the same code path. > - Shouldn't cleanup_trash be empty_trash in Windows parlance? :) > Sure. > - Couldn't the shutil.rmtree() in cleanup_trash() fail due to the files > there still being in use? > Yes. That is why True is passed for the second argument. The idea here is to keep trying once each team pkg(1) is executed. Eventually the files will get removed. > - line 144: is there a particular errno that gets set when you try to > rename on top of an existing file? > Yes. It's errno 17, EEXIST. I'll switch the code to explicitly check for that. > - line 158: this introduces a race. Perhaps mkdtemp() instead, and put > every file in its own directory? > Yes. Fixed to use a temporary subdirectory instead. > - line 159: "dst" here is the source of the rename to temp, right? In > that light, does the comment make any sense? I thought you could move > a file that was in use, just not remove it (or rename on top of it or > any other file). As long as you don't lose the race, this rename > should always succeed, no? > Windows has three types of locking for files[1]: 1. Using share access controls that allow applications to specify whole-file access sharing for read, write or delete; 2. Using byte range locks to arbitrate read and write access to regions within a single file; and 3. By Windows file systems disallowing executing files from being opened for write or delete access. This fix only deals with files locked with type 3. With the first two types, there is no way to actually do anything at all with the file. Thus, the update is going to fail because an exception is going to be thrown from line 161. I'm planning on opening another bug to deal with the first two types by checking for this condition in the file.preinstall method so that the update can be stopped before it starts. This isn't being fixed with this bug because this bug only deals with being able to update IPS itself and the underlying python that it is using, that file locks of the first two types don't come into play with that scenario. [1] http://en.wikipedia.org/wiki/File_locking > - line 163: there's another potential race here, too. If you care, you > could recurse. Or is two tries before a traceback sufficient? > > - I'd probably wrap all of the inner exception clause here in a single > function, move_to_trash() (or something like that). > sure. > t_plat.py: > > - line 100: This means you're unable to perform the test, which seems > bad. Perhaps it shouldn't return silently? Maybe assert that the file > exists, or have a series of files that you try? > ok. I'll assert the file exists. > - line 111,126: No need for the parens around the LHS. > ok. Tom _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
