On Fri, Dec 21, 2007 at 01:13:32PM -0800, [EMAIL PROTECTED] wrote: > > > http://cr.opensolaris.org/~johansen/pkg-unknown/index.html > > > > This seems overengineered to me, given our current requirements, which are > > pretty minimal. (I have the feeling that you weren't quite sure what to do > > here.) I was thinking more along the lines of putting checks at 552 and > > 606 to make sure that cv was "in 'CV'", and perhaps ensure that pkg == > > "pkg". > > Uh, yes, kindof, but not really? > > The original version of this code simply ignored any catalog entry that > we didn't understand. When we downloaded one, we ignored it, and > omitted it from the catalog. If one happened to sneak into a catalog, > we just ignored it too.
Well, kindof, but not really. :) There were a handful of places where the code just assumed that the type was "C" or "V", and ploughed on through, which would have led to a traceback if the line hadn't had the right number of fields or one of them wasn't proper input to the PkgFmri constructor. That bad behavior was all the bug was intended to cover. > However, if we're downloading incremental updates to the catalog, or any > catalog with entries we don't understand, the client doesn't have enough > information to convert the entry from the server-side format to the > client-side format. We're saving the prefixes that we've seen, or can > convert, so that once we run a client with code that can process a > server-to-client format conversion, we go back and convert the stuff we > didn't know how to handle. I see. That makes sense. > > - line 229ff - other than the calls to added_prefix(), all this is doing > > is setting the "prefix" attribute to "known_prefixes". > > It's adding any new prefixes in known_prefixes to the "prefix" attribute > and writing it back to the attributes file, yes. I'm suggesting that you needn't do all the set arithmetic, and that you just write known_prefixes into the attribute directly, since it ends up doing exactly the same work with less and more readable code. > > - wouldn't we want to check if any of the prefixes in the catalog aren't > > recognized (i.e., "if pfx_set - known_prefixes:")? > > This is a good question. If any prefixes in pfx_set aren't in > known_prefixes, we'll just skip them as most of the code only checks the > known_prefixes list. If something is in pfx_set that isn't in > known_prefixes, it's likely that someone ran an old version of the > client, used a new version, and then used the old version again. I'm > assuming that in this situation we want the older client to be able to > get work done. Okay. > > - line 546ff - so I would have just done this first test after the > > split() on line 551 -- "if cv not in known_prefixes:". But the check > > should actually be more specific here, because this bit of the code can > > only handle "C" and "V" types. In fact, adding just that check was > > pretty much the intent of this bug. > > I don't think I understand. In what way should this check be more > specific? On line 551, you split() the entry into a 4-tuple. You're adding the check that the second element is "pkg", which is good, but you should also be checking that the first element is either "C" or "V". If it's, say, "Z" (and "Z" is in known_prefixes), we carry on, and potentially screw up. For that matter, assuming a 4-tuple is probably wrong, too. Assume a 2-tuple, instead, limiting the split to two, which will give you either a 2- or 3-tuple. The third element of that tuple, if it exists, can be split further later on. > > - Similar comment for the hunk at line 600, though here you don't do a > > check inside the try clause, but you do in the except, which is > > confusing. > > I took a look at this code, and I'm confused by this comment. The check > against known_prefixes happens outside of the try both here and at line > 546. The check if pkg == "pkg" is in the try in both cases. Sorry I was confusing. The check you make on 614 (in the except) is not duplicated in the try block. And it's that check which should also be in the previous similar code, back in the 550s, in both the try and the except blocks in that case, too. And I think the isspace or in known_prefixes test isn't useful with this tighter test. > > updatelog.py: > > > > - it occurs to me that we don't have any documentation of what the format > > of an update log looks like, the way that Catalog's docstring does. > > I'll add this, but it's basically + or -, a timestamp, and then the > catalog entry in serverside-format. Yep, but it's good to have formats documented. Thanks. > > - It's probably worth putting a XXX comment in here to at least log a > > warning when we retrieve an update log that contains an entry we don't > > understand. > > I'm not sure it's a great idea to emit a warning every time we read a > catalog line we don't understand. Do we really want clients doing that > when we've introduced a bunch of new catalog types? I'm worried that a new catalog type might mean that a client will have to (or be strongly encouraged to) do something special (like upgrade the client), but if they're never told, they're never gonna know. The question is how urgent such action -- and thus such notification -- is, or might be. It's an open question, I think, so an XXX comment seems appropriate. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
