Shawn Walker wrote: > On 03/ 1/10 07:05 PM, Danek Duvall wrote: > >Shawn Walker wrote: > > > >>http://cr.opensolaris.org/~swalker/pkg-529/ > > > > - if makedirs() blows up not because the leaf directory isn't a > > directory, but because something up the chain isn't, what happens? > > Probably not what I want to happen. I could compare e.filename to path > at the beginning and simply re-raise immediately if it doesn't match? > (Could do that anywhere we call makedirs in the actions I'm changing > really...).
You could do it in makedirs itself, too, no? > > - Shouldn't the logic in file.py to prevent installing through a > > symlink be used here as well? > > Hadn't thought of it; I'll add it. Which reminds me, should I continue > to do the realpath check in install(), or should I do it in preinstall()? > I figured it was better to do in install, but I leave it up to you. I'd figured that we would eventually end up with a phase running prior to any action removal/update/install that would validate the image against what we expected to be there -- not a full verify step, but something that would go and check to make sure that directories hadn't turned into symlinks by accident, and the like. Having that test during install is still important, but if we can give users a heads-up that the plan will fail on execution, or even tell them so with pkg image-update -n, then it's useful in a different way than catching the problems reactively. So the answer to your question, roughly, is "we want both". > >file.py: > ... > > - line 437: why the islink() test? > > Because os.path.isdir() will return true if it's a link pointing at a > directory. In that case, the error for unlink is saying that the > user doesn't have permission to remove the link. Okay. That shouldn't be an issue if you just do the lstat() once and look at the results of S_ISDIR() and S_ISLNK(). Note that if os.path.isdir() calls stat() instead of lstat(), then you're using it incorrectly to make the decision on line 2512 of image.py. There may be other places this is happening, too. > > - line 441, 443: why the existing exception in one case, and an > > ActionExecutionError in the other? > > Because EPERM gets translated into ApiPermissionsException by the > imageplan and any other error we've historically translated into > ActionExecutionError. I don't know why we have ActionExecutionError > really... The intent was to wrap exceptions raised during the execution of an action so that the top level could distinguish them from exceptions raised in other parts of the code, and emit sane errors about a particular action causing a problem. It only got implemented in a few places, though. If you want separate exception classes for "something went wrong during action execution" and "the image didn't match what we expected during action execution", then I'd name the latter ImageStateException (or Error; it's not clear to me how to choose between the two). I'm not sure why you'd want one to be an API exception and the other not. > >image.py: > > > > - line 2485: while I understand lstripping "/" from path, I'm not > > sure I understand stripping self.root. > > Because if the path to the image is /tmp/ips.test.3245/image/, I > don't think you want the salvaged entry (e.g. /my/fancy/file) to be > stored as: > > /tmp/ips.test.3245/image/var/pkg/lost+found/tmp/ips.test3245/image/my/fancy/file > > ...I think you want: > > /tmp/ips.test.3245/image/var/pkg/lost+found/my/fancy/file > > In other words, stored salvage paths should be relative to the image > root since the image itself could move around if the image.root != > '/'. Ah. And salvagedir() is (or can be) called with the full path, rather than just the path relative to the image root. This hasn't been the case so far, has it? > >t_api_search.py: > > > > - test_bug_8318: is this a mis-merge, or some other interaction with > > Brock's change? > > Intentional changes for 14940. It just makes the test suite faster > and chopped a lot of code out. Otherwise, it would have been going > through API object creation twice. There are probably other places > where this could be done with more work, but this was the simplest > test to update. I'm talking about the call to time.sleep() and the change in num_expected, neither of which seem to have much to do with 14940. You explained the sleep call in your other message, but I still don't get the change to num_expected. > >t_pkg_install.py: > > > > - line 4931: this isn't ever used -- test seems incomplete > > Intentionally incomplete; the comment for the test is misleading. The > happy medium I took with the cli test suite was that it only tests for > failure cases (that the client doesn't traceback) and assumes that all > the success cases will be handled by the API tets (the docstring for the > test suite says so). Okay, but there's no need to create variables that aren't ever used. I like this set of fixes, but I'm uncomfortable putting them into a respin of the last build of the release. There are too many changes in finicky parts of the code, and this just needs more soak time before it can go into the release. I'd be much happier targeting this for a backport to 2010.03 in a couple of months (or whenever we start opening the 2010.03 gate for that kind of work). Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
