Shawn Walker wrote: > On 07/26/11 16:08, Danek Duvall wrote: > >Shawn Walker wrote: > > > >>https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-validate-1-2/webrev/ > > > >generic.py: > > > > - line 1137: why not a try/except here? Just not in the fast path? > > Basically because it's not a hot path that I cared enough about. I > don't mind changing it for consistency if so desired.
No need; just curious. > >imageplan.py: > > > > - line 2983: Why wedge this into ActionExecutionError? Wouldn't a new > > SymlinkLoopError be better? That way we can use it in other parts of > > the code that might run into the same problem. > > Because the client already handles ActionExecutionErrors gracefully > and I didn't want clients to have to be updated to handle that. Yes, > I could make it a subclass of ActionExecutionErrors, but I also felt > like it wasn't a case that clients actually needed to specifically > detect and handle. It was good enough to simply say what went wrong. Okay; I don't think this will be a common occurrence. > > - line 2985: The link needn't target itself; only be part of a loop, > > which might be much longer than a single path. (I don't think you need > > to add tests for this, but the error text could be broader.) > > I've attempted to clarify the error text: > > "A link targeting itself or part of a link loop was found at '%s'; a > file or directory was expected. Please remove the link and try > again." Sounds fine. > > - line 2988: will simply removing the link work, or would you have to run > > pkg fix first (assuming the path in question isn't supposed to be > > changing in this plan)? > > That depends on the scenario. If it's supposed to be a file, simply > removing the link should be sufficient since we failed while > attempting to deliver something to the same location. However, if it > was a directory, then yes, pkg fix could potentially be the only > correct way to fix it (after removing it currently). > > >t_pkg_install.py: > > > > - Does pkg fix properly remove a link targeting itself and replace it > > with whatever should be there? > > No, because we currently rely on the install path to perform our > fixes. So fix would exit with the same error, but it would restore > the item properly if you removed the bad link first. > > Getting fix to be automatically repair it would be neat, but is > definitely non-trivial. I'm uncertain if I should expend that much > effort on this particular case. I can either file a bug for us to > eventually do that, or pass on this for now. Don't bother fixing it now, but filing a bug (perhaps with a patch adding a test case demonstrating the problem) would be nice. Thanks, Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
