Danek Duvall wrote: > On Wed, Jan 07, 2009 at 01:08:03PM -0600, Shawn Walker wrote: > >>> catalog.py: >>> >>> - line 192: Why distinguish EPERM from all other non-ENOENT errors? >> Because EPERM can happen when the user is running the pkg client without >> the necessary privileges to chmod the file. That's why I then go and check >> to see if the privileges actually need to be fixed and fail if they're >> wrong if EPERM occurs. >> >> The other alternative is to simply ignore EPERM as I do ENOENT and assume >> the best. > > Sorry if I wasn't clear -- why would you treat any other failure any > differently than you do EPERM? Your ENOENT handling makes sense, but why > the second if?
Oh. You're probably right, I could just assume all other errors should get the same treatment as EPERM. I've dropped the if e.errno == errno.EPERM qualifier. >>> - line 817: I don't really see why you're calling this here. You only >>> need to make sure that perms are correct on the attrs file, but they >>> will be, given that you called __set_perms() on startup, and that you >>> never create it anew with mkstemp() (which probably should be fixed, >>> but not here). >> The attrs file might not exist until save_attrs() gets called (which >> doesn't happen until check_prefix() gets called later during __init__). >> So, the right solution seems to be to move the __set_perms call to the very >> end of __init__ and drop the __set_perms call from save_attrs(). > > Okay. Though if you're creating the attrs file correctly, then it doesn't > matter -- __set_perms() is used only to correct previous versions of the > depot which didn't create the files with the correct perms, and correct > usage of chmod takes care of any time the files are created. We don't do a chmod() right now for the attrs file after it's created since (unlike everything else) we don't create a temporary file first; it opens the file in wb+ mode. I suppose this means that there is a race condition for the attrs file too just as there was for the catalog files. I figured that since the chmod needs to happen during __init__ anyway, I might as well rely on it to setup the correct permissions for the attrs file. What do you want to see here? >> Updated webrev: >> http://cr.opensolaris.org/~swalker/pkg-5603-4/ > > You needn't change recv() to be a classmethod if you refer to file_mode as > Catalog.file_mode, but as long as you're confident that the change from > staticmethod to classmethod is safe, I don't care too much. The test suites pass, and I didn't see anything concerning in the Python docs. I just preferred that over self-referencing the class directly since this way still lets inheritance work as expected. Cheers, -- Shawn Walker _______________________________________________ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
