On Thu, Oct 23, 2008 at 03:25:24PM -0700, [EMAIL PROTECTED] wrote: > > - One small suggestion I'd have for recv() is to do as you were doing > > before -- writing the file line by line, rather than buffering > > everything up -- but writing into a temporary file, and only moving > > that temporary file into place after you check that the transfer was > > complete. That'd help the method be more atomic, anyway. I'm not sure > > there's any point in keeping the temporary file around if it's short, > > though maybe you can think of one. > > Short updates should be happing through the updatelog. I don't see a > problem with the approach you've suggested.
I was being too terse (short). I meant that if the catalog data you got was shorter than you expected, I couldn't think of any reason you'd want not to delete the temporary file, but maybe you could think of one. > > - line 721: We have a distinction between server methods which stream > > their data and those that can send everything over the wire at once > > (more or less). Sending content-size now requires that send() be the > > latter. Does this mean any other plumbing work? > > As far as I can tell, there aren't any further plumbing changes > required. It should be legit to sent a Content-Length header, even if > we're streaming. We've always been able to compute the length of the > transfer, but just never bothered. I believe we want to continue to > stream these operations so as to keep the memory footprint reasonable. Okay. As long as Shawn doesn't think there are any other changes needed -- like re-decorating the catalog handler or something. > > filelist.py: > > > > - line 379: is there any need for this test? It seems that it's > > irrelevant given the test on the next line. (And there are a bunch of > > other tests as this general structure is repeated.) > > This test is here because I'm paranoid. I took a look at the code for > Python-2.4.4/Modules/socketmodule.c > > In that code, there seem to be three different ways that socket.error > (socket_error in the C code) gets set. One case uses PyErr_SetString(), > the other uses set_error() in the module and then Py_BuildValue's an > exception as an integer, string tuple. The third case uses > PyErr_SetFromErrno(). That function also creates an integer, string > tuple. This seems to imply that if we ever get a tuple as the argument > to a socket.error exception, the first argument should be an integer. I > thought we might want to double-check, should this change. However, if > you think this test is superfluous, I'd happy to remove it. I think it's more that your set has three objects in it, and they're integers, so if you give it some other object, it will by definition not be found in that set, so the next test will be false. > > - line 518: the InvalidContentException constructor expects its second > > argument to be "hashval", not, um, whatever this is. Same with lines > > 527 and 541. Perhaps the constructor should be enhanced to take an > > expected and actual hash value, and a ... something for the line 518 > > case, and have the __str__() method do the appropriate thing, rather > > than generating the message at this level? > > I actually changed this in misc.py, since I'm using the > InvalidContentException to handle both invalid hashes and invalid > compressed content. The exception was changed to take a string as the > second argument, and I've been generating depending upon what failed. > > Are you objecting to this overall approach, or just concerned that I'm > not passing the right arguments? If it's the latter, this was fixed in > misc.py. Overall approach -- I much prefer to create exceptions with raw data, and have the exceptions or their handlers figure out what the messaging around them should be, rather than passing the exception a mostly cooked message. I don't care too much here, but will note that it'll probably to change down the line. > > - line 151: This should be in a finally clause. > > None of the other exception handlers add a try/finally. Until we switch > to a version of python >= 2.5, try/finally requires a separate block. Oh, right; crap. > Adding another level of indentation to that code is going to make it > even harder to read. The other retrieval methods don't perform a close > at all and let the object be implicitly closed as it goes out of scope. > Would you prefer that I do that instead? Okay, that's probably good enough. <sigh> Same for the manifest version. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
