Shawn Walker wrote: > On 07/19/11 09:49, Danek Duvall wrote: > >Shawn Walker wrote: > > > >>https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-validate-1/webrev/ > > > >depend.py: > > > > - line 100: why keep a check for "type", but not for "fmri"? > > Because 'fmri' is a key attribute so will be handled by generic.__init__(). > > However, 'type' is required for the depend action to be meaningful > and is not handled by the key attribute check. > > If you think that it's ok if we only check type at publication/pkglint > time for depend actions, I can move this to _validate().
No, this is fine. > >generic.py: > > > > - line 201: why this change? The get() call is slower than the exception > > block setup? > > Yes, by about 2% consistently; I've added a comment. Okay. I'll try to remember that, though; we shouldn't have to comment this kind of construction every time if it's considered part of the usual paradigm. > >link.py: > > > > - line 63-66: meh. There's nothing wrong with creating such a beast, > > just like there's nothing wrong with creating dangling symlinks; we > > simply shouldn't die when trying to traverse it. And I don't think I > > saw any code that would prevent that. > > It's true I didn't add an errno catch, which I'll go do, but I *did* add > a check that raises an error if target and path are the same which I'll > remove in favour of the errno check. > > Although I sort of don't see the point of allowing them; it's going to > cause operation failure since we can't successfully install the link. Why wouldn't we be able to install the link? You can create symlink loops just fine, and there's nothing about the packaging system that would make you unable to do so. The problem occurs when you try to *traverse* the loop. We hit the original bug because a path was installed as a symlink instead of a file (installer shenanigans) and the symlink pointed to itself mistakenly. Because the packaging system thought it was a file, it thought chmodding it would make sense, and when trying to do so, the VFS traversed the link and failed with ELOOP. In fact, I believe that we can deliver and install symlink loops and always operate just fine, because we'll never try to traverse the loop -- as long as we know that those actions are links. The problem should only happen when someone goes and mucks with the filesystem and changes a file or a directory to a symlink and then we go try to do something with it assuming it is what it should be. This is a "something changed out from under us" bug, not a "bad package" bug. So in order to fix the bug, we need to catch ELOOP at a higher level, where we catch other filesystem problems. Also, note that preventing links from pointing to themselves doesn't prevent any other loops, which are just as easy to create. > >signature.py: > > > > - line 478: What about this code matches the comment? Or is the comment > > just a warning of some sort? > > It's my attempt to document that "value" is checked for in validate() > even though it's a key attribute. In all other cases, we require the > key attribute at action.__init__(), but this case requires a special > exception because of how signing uses signature actions. Okay. I guess I don't understand why the comment says (as far as I understand it) "you don't have to have a value attribute during publication" and then proceeds to emit an error if you don't have it without checking if we're in publication. > > - line 481: 'if "value" not in self.attrs'? > > Because you can specify value="" on an action and that's not enough. > But regardless, I've switched this over to using required_attrs on > _validate(). If you want to enforce a non-empty value attribute specially, that's fine, but I'd guess that it actually needs more validation than that in the end. > >dependencies.py: > > > > - line 1352: why treat this as a keyword arg? > > Because it is? Everywhere else, we treat it as an optional positional argument. In fact, one of the places where you added the argument you didn't bother with the keyword. > ... > >t_pkgsend.py: > > > > - line 118: what's wrong with this manifest? > > Nothing is wrong with the manifest, but originally, testing this > required providing a manifest as an argument. I've removed the > manifest bit now. So what causes pkgsend to die here? Is it the lack of a repo to publish to? Thanks, Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
