Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Wed, Jan 07, 2009 at 04:29:49PM -0600, Shawn Walker wrote: > >> Danek Duvall wrote: >>> On Wed, Jan 07, 2009 at 03:02:51PM -0600, Shawn Walker wrote: >>> http://cr.opensolaris.org/~swalker/pkg-5603-4/ >>> Isn't this what I reviewed before? I think that was fine -- my only real >>> comment was the EPERM if statement, which you said you removed (though it >>> still shows up here). >> Sorry I rsync'd the wrong webrev: >> http://cr.opensolaris.org/~swalker/pkg-5603-4/ > > The EPERM change is the only one since the previous -4? Looks fine to me. Yes. Thanks; I'll putback now. -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Wed, Jan 07, 2009 at 04:29:49PM -0600, Shawn Walker wrote: > Danek Duvall wrote: >> On Wed, Jan 07, 2009 at 03:02:51PM -0600, Shawn Walker wrote: >> >>> http://cr.opensolaris.org/~swalker/pkg-5603-4/ >> >> Isn't this what I reviewed before? I think that was fine -- my only real >> comment was the EPERM if statement, which you said you removed (though it >> still shows up here). > > Sorry I rsync'd the wrong webrev: > http://cr.opensolaris.org/~swalker/pkg-5603-4/ The EPERM change is the only one since the previous -4? Looks fine to me. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Wed, Jan 07, 2009 at 03:02:51PM -0600, Shawn Walker wrote: > >> http://cr.opensolaris.org/~swalker/pkg-5603-4/ > > Isn't this what I reviewed before? I think that was fine -- my only real > comment was the EPERM if statement, which you said you removed (though it > still shows up here). Sorry I rsync'd the wrong webrev: http://cr.opensolaris.org/~swalker/pkg-5603-4/ Look again. Thanks, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Wed, Jan 07, 2009 at 03:02:51PM -0600, Shawn Walker wrote: > http://cr.opensolaris.org/~swalker/pkg-5603-4/ Isn't this what I reviewed before? I think that was fine -- my only real comment was the EPERM if statement, which you said you removed (though it still shows up here). Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Wed, Jan 07, 2009 at 01:40:04PM -0600, Shawn Walker wrote: > >>> Sure, that's fine. I just wanted to point out that wasn't necessary. >> It is if their umask is wrong. > > We get a lot of stuff wrong if the umask is wrong, not just this. So it is ok now? http://cr.opensolaris.org/~swalker/pkg-5603-4/ -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Wed, Jan 07, 2009 at 01:40:04PM -0600, Shawn Walker wrote: >> Sure, that's fine. I just wanted to point out that wasn't necessary. > > It is if their umask is wrong. We get a lot of stuff wrong if the umask is wrong, not just this. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
> I was thinking that since updatelog files kept the changes to the catalog > compared to a certain base, you could just make changes to the updatelog > and not the catalog, but I guess the catalog file is intended to hold the > full catalog data at all times, anyway. > > It might be better still to only write the catalog out periodically when > getting a lot of updates via add_fmri(), if we think that rewriting the > catalog on each update is hurting performance. Stephen and I had discussed this approach during the original development of the updatelog. At the time, we considered it overly complex for the purpose of supplying incremental updates. The atomicity requirement for the catalog was added later. In practice, the approach that you've described starts to look a lot like the transaction log for a database. Instead of replacing the catalog, we get to write the transaction log and sync it to disk every time we commit a transaction. In our current model, each transaction results in the creation of a FMRI. I'm not sure that there's a huge difference in performance for a create-modify-rename operation compared to a situation where we write and then flush a transaction log, followed by a write and flush of the catalog. Of course, as Bart likes to say, one good experiment is worth a hundred expert opinions. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Wed, Jan 07, 2009 at 01:26:35PM -0600, Shawn Walker wrote: > >> 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. > > Right. If we were to use a temporary file, the chmod would be necessary. > It's correct as is. > >> I suppose this means that there is a race condition for the attrs file too >> just as there was for the catalog files. > > How? The file is just created mode 644 to start, never any worry. The ...unless the user's umask is set incorrectly. umask 0066, for example, causes the attrs file to get 600 instead of 644 permissions when the attrs file gets created during startup for a new repository. >> 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. > > Sure, that's fine. I just wanted to point out that wasn't necessary. It is if their umask is wrong. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Wed, Jan 07, 2009 at 01:26:35PM -0600, Shawn Walker wrote: > 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. Right. If we were to use a temporary file, the chmod would be necessary. It's correct as is. > I suppose this means that there is a race condition for the attrs file too > just as there was for the catalog files. How? The file is just created mode 644 to start, never any worry. The only thing that's wrong here is that the attrs file is written to directly, meaning that it could be served up in the middle of the write. > 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. Sure, that's fine. I just wanted to point out that wasn't necessary. > 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. Okay. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
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
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
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? >> - 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. > 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. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Wed, Jan 07, 2009 at 09:15:52AM -0600, Shawn Walker wrote: > >>> Besides, isn't this one of the things we could use the updatelog for? >>> Seems like an excellent opportunity to get the two tied together a bit >>> better. >> Not sure I see the connection at the moment, but that's for another day >> anyway. > > I was thinking that since updatelog files kept the changes to the catalog > compared to a certain base, you could just make changes to the updatelog > and not the catalog, but I guess the catalog file is intended to hold the > full catalog data at all times, anyway. > > It might be better still to only write the catalog out periodically when > getting a lot of updates via add_fmri(), if we think that rewriting the > catalog on each update is hurting performance. It's something I'd like to look at soon, so your suggestion is incredibly helpful. I hadn't thought of that approach with the updatelog. >> So is the latest changeset ok with you then? >> >> http://cr.opensolaris.org/~swalker/pkg-5603-3/ > > 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. > - 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(). Updated webrev: http://cr.opensolaris.org/~swalker/pkg-5603-4/ diff from last webrev: http://cr.opensolaris.org/~swalker/pkg-5603-4/v3-v4.patch Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Wed, Jan 07, 2009 at 09:15:52AM -0600, Shawn Walker wrote: > http://cr.opensolaris.org/~swalker/pkg-5603-3/ And don't forget copyright statement updates. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Wed, Jan 07, 2009 at 09:15:52AM -0600, Shawn Walker wrote: >> Besides, isn't this one of the things we could use the updatelog for? >> Seems like an excellent opportunity to get the two tied together a bit >> better. > > Not sure I see the connection at the moment, but that's for another day > anyway. I was thinking that since updatelog files kept the changes to the catalog compared to a certain base, you could just make changes to the updatelog and not the catalog, but I guess the catalog file is intended to hold the full catalog data at all times, anyway. It might be better still to only write the catalog out periodically when getting a lot of updates via add_fmri(), if we think that rewriting the catalog on each update is hurting performance. > So is the latest changeset ok with you then? > > http://cr.opensolaris.org/~swalker/pkg-5603-3/ catalog.py: - line 192: Why distinguish EPERM from all other non-ENOENT errors? - line 651: this blank line makes the previous comment seem like it's missing its code. You might even get rid of the comment on line 652. Or even make file_mode a class member. - 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). Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Tue, Jan 06, 2009 at 08:43:46PM -0600, Shawn Walker wrote: > >> Danek Duvall wrote: >>> On Tue, Jan 06, 2009 at 07:53:46PM -0600, Shawn Walker wrote: >>> That might be best with a few twists; I just realised we re-create the catalog file every time we add an fmri, which is atrocious anyway. >>> Maybe, but it's atomic. If you write the catalog file directly, there's a >>> window where it may be served improperly formatted. Not to mention if you >>> accidentally have two depots running in the same repo. >> Right, I know why we did it, but it screams slow. >> >> I wonder just how much it affects publishing performance... > > Please prefer correctness over performance. If you don't know how much it > affects performance, don't make the change until you've measured it. Right, which is why I didn't change what we were doing. > Besides, isn't this one of the things we could use the updatelog for? > Seems like an excellent opportunity to get the two tied together a bit > better. Not sure I see the connection at the moment, but that's for another day anyway. So is the latest changeset ok with you then? http://cr.opensolaris.org/~swalker/pkg-5603-3/ Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Tue, Jan 06, 2009 at 08:43:46PM -0600, Shawn Walker wrote: > Danek Duvall wrote: >> On Tue, Jan 06, 2009 at 07:53:46PM -0600, Shawn Walker wrote: >> >>> That might be best with a few twists; I just realised we re-create the >>> catalog file every time we add an fmri, which is atrocious anyway. >> >> Maybe, but it's atomic. If you write the catalog file directly, there's a >> window where it may be served improperly formatted. Not to mention if you >> accidentally have two depots running in the same repo. > > Right, I know why we did it, but it screams slow. > > I wonder just how much it affects publishing performance... Please prefer correctness over performance. If you don't know how much it affects performance, don't make the change until you've measured it. Besides, isn't this one of the things we could use the updatelog for? Seems like an excellent opportunity to get the two tied together a bit better. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Tue, Jan 06, 2009 at 07:53:46PM -0600, Shawn Walker wrote: > >> That might be best with a few twists; I just realised we re-create the >> catalog file every time we add an fmri, which is atrocious anyway. > > Maybe, but it's atomic. If you write the catalog file directly, there's a > window where it may be served improperly formatted. Not to mention if you > accidentally have two depots running in the same repo. Right, I know why we did it, but it screams slow. I wonder just how much it affects publishing performance... -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Tue, Jan 06, 2009 at 06:01:36PM -0800, johan...@sun.com wrote: > I'll forgive you for confusing the two of us, but I'm not sure if he > will. ;) I will if you dye your hair and grow it out. ;-) Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Tue, Jan 06, 2009 at 07:53:46PM -0600, Shawn Walker wrote: > That might be best with a few twists; I just realised we re-create the > catalog file every time we add an fmri, which is atrocious anyway. Maybe, but it's atomic. If you write the catalog file directly, there's a window where it may be served improperly formatted. Not to mention if you accidentally have two depots running in the same repo. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
> That's essentially what I've ended up doing. I've left the call to > __set_perms for the __init and save_attrs case which all happens once > generally during load. I've changed the cases where we do the tempfiles > to chmod them before the rename Ok, great. Thanks for changing that. > thanks for pointing out the race case earlier. It was dduvall who pointed that out. I'll forgive you for confusing the two of us, but I'm not sure if he will. ;) -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
johan...@sun.com wrote: >> The code in recv() is changing permissions on the temporary files, not >> the final files that are in place. > > The permissions are not changed when a rename occurs. I'm calling chmod > on the tempfile, but when the rename occurs the new files will have 644 > mode. > > This is why I asked if it made sense to set the mode on your tempfiles > and then just rename them. If you're trying to repair files with > incorrect permissions, it might make sense to break that into a separate > step. I.e. set the mode for the tempfiles before the rename, and then > have a repair routine that you run in the few cases where users/depots > have leftover bad permissions. Assuming that the software performing > the chmod has the permissions to do so, of course. That's essentially what I've ended up doing. I've left the call to __set_perms for the __init and save_attrs case which all happens once generally during load. I've changed the cases where we do the tempfiles to chmod them before the rename; thanks for pointing out the race case earlier. -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Tue, Jan 06, 2009 at 06:22:39PM -0600, Shawn Walker wrote: > >> I'm not sure what to do here then; I can't see an os.stat() being done for >> every single add_fmri being great for publishing performance. > > Well, you could just chmod the files every time, without doing any stats. > As it is, you're already doing two stats on each file, assuming it exists. That might be best with a few twists; I just realised we re-create the catalog file every time we add an fmri, which is atrocious anyway. The stat is probably the least of our performance concerns. I've also addressed the race condition comment you and johansen pointed out and modified the test case to account for permissions checking after publishing. Updated webrev: http://cr.opensolaris.org/~swalker/pkg-5603-3/ Sorry, I have no diff from last webrev due to a recommit faux pas on my part. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
> The code in recv() is changing permissions on the temporary files, not > the final files that are in place. The permissions are not changed when a rename occurs. I'm calling chmod on the tempfile, but when the rename occurs the new files will have 644 mode. This is why I asked if it made sense to set the mode on your tempfiles and then just rename them. If you're trying to repair files with incorrect permissions, it might make sense to break that into a separate step. I.e. set the mode for the tempfiles before the rename, and then have a repair routine that you run in the few cases where users/depots have leftover bad permissions. Assuming that the software performing the chmod has the permissions to do so, of course. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
johan...@sun.com wrote: >>> This might be an inane question, but why bother with __set_perms() at >>> all? Can you just chmod the tmpfile to 644 before you rename it into >>> the catalog directory? >> It was done to centralise the permissions logic in one place since there >> are multiple places that catalog files are created and used. > > Okay, but it looks like you haven't finished the centralization. The > code in recv() dorks with the permissions on the client side, but it > looks like that code is unchanged. > > Did you mean that you're centralizing the depot portions of the > permissions management? The code in recv() is changing permissions on the temporary files, not the final files that are in place. I was centralising the logic for the final files (per a comment from Brad); not the temporary files. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
> > This might be an inane question, but why bother with __set_perms() at > > all? Can you just chmod the tmpfile to 644 before you rename it into > > the catalog directory? > > It was done to centralise the permissions logic in one place since there > are multiple places that catalog files are created and used. Okay, but it looks like you haven't finished the centralization. The code in recv() dorks with the permissions on the client side, but it looks like that code is unchanged. Did you mean that you're centralizing the depot portions of the permissions management? -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Tue, Jan 06, 2009 at 04:35:30PM -0800, johan...@sun.com wrote: > > I'm not sure what to do here then; I can't see an os.stat() being done > > for every single add_fmri being great for publishing performance. > > This might be an inane question, but why bother with __set_perms() at > all? Can you just chmod the tmpfile to 644 before you rename it into > the catalog directory? That raises another good point -- right now the chmod happens after the rename, which means there's a window of 0600 exposure. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
johan...@sun.com wrote: >> I'm not sure what to do here then; I can't see an os.stat() being done >> for every single add_fmri being great for publishing performance. > > This might be an inane question, but why bother with __set_perms() at > all? Can you just chmod the tmpfile to 644 before you rename it into > the catalog directory? It was done to centralise the permissions logic in one place since there are multiple places that catalog files are created and used. -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
> I'm not sure what to do here then; I can't see an os.stat() being done > for every single add_fmri being great for publishing performance. This might be an inane question, but why bother with __set_perms() at all? Can you just chmod the tmpfile to 644 before you rename it into the catalog directory? -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Tue, Jan 06, 2009 at 06:22:39PM -0600, Shawn Walker wrote: > I'm not sure what to do here then; I can't see an os.stat() being done for > every single add_fmri being great for publishing performance. Well, you could just chmod the files every time, without doing any stats. As it is, you're already doing two stats on each file, assuming it exists. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Shawn Walker wrote: > Danek Duvall wrote: >> Right. The case I was suggesting was where you start up with both >> files in >> place, __set_perms() runs and disables itself at startup, then add_fmri() >> gets called, and the __set_perms() call doesn't do anything, even though >> you've created the file that will become the catalog file with mkstemp(), >> which creates it with the wrong mode. > > No, it doesn't; because only one of the files gets created at startup. > There should be a test case for this in the unit tests I wrote. Sorry, I just realised I misunderstood you. I'm not sure what to do here then; I can't see an os.stat() being done for every single add_fmri being great for publishing performance. Either way, I need a test case. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Tue, Jan 06, 2009 at 05:47:11PM -0600, Shawn Walker wrote: > >> Danek Duvall wrote: >>> On Mon, Dec 22, 2008 at 08:16:52PM -0600, Shawn Walker wrote: >>> http://cr.opensolaris.org/~swalker/pkg-5603-2/ >>> catalog.py: >>> >>> - line 137: rather than setting this "done" flag, you could, in >>> __set_perms(), redefine __set_perms() to an empty function: >>> >>> self.__set_perms = lambda: None >>> >>> Of course, if you ever think you need to call __set_perms() again for >>> real, then this wouldn't work. >> It is possible at the moment for __set_perms to be called multiple times >> during a single execution. This happens, for example, when you start a >> repository for the first time with an empty directory. The catalog attrs >> file gets created at startup, but the actual catalog file doesn't until the >> first fmri gets published. > > Right, but you never reset the flag to False once it's been set to True. But, when I set the flag to True, I only do so if I've checked both files which only happens if the file exists. See line 192. >>> - line 184, 185: shouldn't you use S_IMODE() here, too? >> No, these lines are to report the modes I already obtained from S_IMODE() >> earlier or am expecting, unless I'm missing something. > > So why are you masking them with 0777? That's what S_IMODE() does > (actually, it does 0, which is probably more correct), but you probably > don't need the masking at all. Hrm. The example I found stated I had to do so. It appears you're right though, I don't need the masking at all. >>> - line 248: this won't do anything if the catalog was initialized when >>> both files were there, which is most of the time, right? Same with >>> the >>> other place you call __set_perms() other than the constructor? >> See above. > > Right. The case I was suggesting was where you start up with both files in > place, __set_perms() runs and disables itself at startup, then add_fmri() > gets called, and the __set_perms() call doesn't do anything, even though > you've created the file that will become the catalog file with mkstemp(), > which creates it with the wrong mode. No, it doesn't; because only one of the files gets created at startup. There should be a test case for this in the unit tests I wrote. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Tue, Jan 06, 2009 at 05:47:11PM -0600, Shawn Walker wrote: > Danek Duvall wrote: >> On Mon, Dec 22, 2008 at 08:16:52PM -0600, Shawn Walker wrote: >> >>> http://cr.opensolaris.org/~swalker/pkg-5603-2/ >> >> catalog.py: >> >> - line 137: rather than setting this "done" flag, you could, in >> __set_perms(), redefine __set_perms() to an empty function: >> >> self.__set_perms = lambda: None >> >> Of course, if you ever think you need to call __set_perms() again for >> real, then this wouldn't work. > > It is possible at the moment for __set_perms to be called multiple times > during a single execution. This happens, for example, when you start a > repository for the first time with an empty directory. The catalog attrs > file gets created at startup, but the actual catalog file doesn't until the > first fmri gets published. Right, but you never reset the flag to False once it's been set to True. >> - line 184, 185: shouldn't you use S_IMODE() here, too? > > No, these lines are to report the modes I already obtained from S_IMODE() > earlier or am expecting, unless I'm missing something. So why are you masking them with 0777? That's what S_IMODE() does (actually, it does 0, which is probably more correct), but you probably don't need the masking at all. >> - line 248: this won't do anything if the catalog was initialized when >> both files were there, which is most of the time, right? Same with >> the >> other place you call __set_perms() other than the constructor? > > See above. Right. The case I was suggesting was where you start up with both files in place, __set_perms() runs and disables itself at startup, then add_fmri() gets called, and the __set_perms() call doesn't do anything, even though you've created the file that will become the catalog file with mkstemp(), which creates it with the wrong mode. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Danek Duvall wrote: > On Mon, Dec 22, 2008 at 08:16:52PM -0600, Shawn Walker wrote: > >> http://cr.opensolaris.org/~swalker/pkg-5603-2/ > > catalog.py: > > - line 137: rather than setting this "done" flag, you could, in > __set_perms(), redefine __set_perms() to an empty function: > > self.__set_perms = lambda: None > > Of course, if you ever think you need to call __set_perms() again for > real, then this wouldn't work. It is possible at the moment for __set_perms to be called multiple times during a single execution. This happens, for example, when you start a repository for the first time with an empty directory. The catalog attrs file gets created at startup, but the actual catalog file doesn't until the first fmri gets published. > - line 184, 185: shouldn't you use S_IMODE() here, too? No, these lines are to report the modes I already obtained from S_IMODE() earlier or am expecting, unless I'm missing something. > - line 248: this won't do anything if the catalog was initialized when > both files were there, which is most of the time, right? Same with the > other place you call __set_perms() other than the constructor? See above. Everything else you mentioned I've changed. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Oh, and note that since you made all these changes back in 2008, you don't need to change the copyright statements to 2009 just because you're doing the commit / push in 2009. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
On Mon, Dec 22, 2008 at 08:16:52PM -0600, Shawn Walker wrote: > http://cr.opensolaris.org/~swalker/pkg-5603-2/ catalog.py: - line 137: rather than setting this "done" flag, you could, in __set_perms(), redefine __set_perms() to an empty function: self.__set_perms = lambda: None Of course, if you ever think you need to call __set_perms() again for real, then this wouldn't work. - line 184, 185: shouldn't you use S_IMODE() here, too? - line 189: why len()? - line 248: this won't do anything if the catalog was initialized when both files were there, which is most of the time, right? Same with the other place you call __set_perms() other than the constructor? t_catalog.py: - line 104: "Catalog". - line 129: couldn't you use assertRaises()? Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Shawn Walker wrote: > Shawn Walker wrote: >> Greetings, >> >> The following webrev contains fixes for the following issues: >> >>5603 server catalog permissions should be 644, not 600 > > updated webrev: > http://cr.opensolaris.org/~swalker/pkg-5603-2/ > > Change Summary: > * Consolidated permissions set code > > * Changed to only check and set permissions once to avoid publishing > performance degradation. > > * Changed to raise an exception if permissions are not correct on > catalog files, but --readonly is in effect. > > * Changed depot to handle above exception and exit with a return code of > 1 displaying the list of files that need their modes corrected. > > * Updated tests to reflect changes. > > Notes: > dp requested the depot exit with an error if the permissions were bad Still need reviewers. Thanks, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Shawn Walker wrote: > Greetings, > > The following webrev contains fixes for the following issues: > >5603 server catalog permissions should be 644, not 600 updated webrev: http://cr.opensolaris.org/~swalker/pkg-5603-2/ Change Summary: * Consolidated permissions set code * Changed to only check and set permissions once to avoid publishing performance degradation. * Changed to raise an exception if permissions are not correct on catalog files, but --readonly is in effect. * Changed depot to handle above exception and exit with a return code of 1 displaying the list of files that need their modes corrected. * Updated tests to reflect changes. Notes: dp requested the depot exit with an error if the permissions were bad Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Brad Hall wrote: > Shawn Walker writes: > >> Greetings, >> >> The following webrev contains fixes for the following issues: >> >>5603 server catalog permissions should be 644, not 600 >> >> webrev: >> http://cr.opensolaris.org/~swalker/pkg-5603/ >> >> Change Summary: >> * Changed modules/catalog.py to force mode to be 644 on catalog files >> unless read_only is True for both new and existing catalog files. >> >> * Added asserts to a few functions that modify the catalog to ensure >> that read_only isn't True. >> >> * Added tests to verify mode is set properly for both cases for catalog >> files. > > catalog.py: > > - We've now got the mode (stat.S_IRUSR|...) in a few different places, > maybe a constant would be cleaner? Alternatively, turning those few > lines into a function could remove even more duplication :) Okay, let me see what I can do. > General comment: > > - What if someone decides that they want their catalog to be 664? I > can't think of a good reason for doing this at the moment. Maybe it > would be less restrictive to just make sure it is group/world readable > combined with whatever mode they have set for it. (i.e. if they have > 0600, you set 0644; if they have 0660, you set 0664, etc.) I'd rather force the permissions to be this way, since that's the known good configuration. Until someone asks for this, I think it's better for the server to be consistent with the client. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] [REVIEW] Fix for bug 5603
Shawn Walker writes: > Greetings, > > The following webrev contains fixes for the following issues: > >5603 server catalog permissions should be 644, not 600 > > webrev: > http://cr.opensolaris.org/~swalker/pkg-5603/ > > Change Summary: > * Changed modules/catalog.py to force mode to be 644 on catalog files > unless read_only is True for both new and existing catalog files. > > * Added asserts to a few functions that modify the catalog to ensure > that read_only isn't True. > > * Added tests to verify mode is set properly for both cases for catalog > files. catalog.py: - We've now got the mode (stat.S_IRUSR|...) in a few different places, maybe a constant would be cleaner? Alternatively, turning those few lines into a function could remove even more duplication :) General comment: - What if someone decides that they want their catalog to be 664? I can't think of a good reason for doing this at the moment. Maybe it would be less restrictive to just make sure it is group/world readable combined with whatever mode they have set for it. (i.e. if they have 0600, you set 0644; if they have 0660, you set 0664, etc.) Thanks, Brad ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
