> > http://cr.opensolaris.org/~johansen/webrev-551/ > > file.py: > > - Now that you're not using ST_MODE and friends, are you able to remove > the "from stat import *" bit at the top?
I removed the import in all the places where I could; however, file.py is still using other parts of stat. I'll change the import to make the calls to stat more obvious. > catalog.py: > > - line 115: this seems to be completely private -- make it __size? I thought adding the _ in front made it private; however, I've made this change. > - line 511: You should document content_size in the docstring. Actually, > content_size is a funny name. Either "content_length" or "size", maybe? I added content_size to the docstring, as requested. > - 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. > - 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. > 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. > - line 392: space before "%d". Similar things in retrieve.py Thanks. Not sure how I missed that. > - 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. > image.py: > > - line 1294: "<="? Why not just "=="? Just paranoia / consistency with the other similar methods. > retrieve.py: > > - line 41: manifest? Thanks. Copy/paste error. > - line 74: "timestap" Thanks, fixed. > - line 125, 149: repr()? Is this just for debugging? Note that you can > use %r instead of %s, and then not have to explicitly call repr(). I assume so. I was imitating the existing error handlers written elsewhere. I'll change these to use %r. > - 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. 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? > - line 390: Do we need to call m.close()? An interesting question, one that depends upon your answer to the previous point, I think. Thanks for reviewing this. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
