On Tue, Apr 26, 2011 at 11:25, Hyrum K Wright <hy...@hyrumwright.org> wrote: > On Tue, Apr 26, 2011 at 12:35 AM, Greg Stein <gst...@gmail.com> wrote: >> On Mon, Apr 25, 2011 at 21:06, Hyrum K Wright <hy...@hyrumwright.org> wrote: >>>... >>> Which kinda irks me, actually. You'd think something as simple as >>> "ensure the permissions on disk match what's in the db" would be >>> fairly straightforward, but it ends up have all kinds of special cases >>> and various shortcuts. In some ways, I guess I'd settle for all of >>> the logic being encapsulated in a single function/set-of-functions, >>> rather than being spread across half-a-dozen functions and two files. >>> >>> As a first step, then, it'd be nice to know what special handling is >>> needed for commit/update/revert, as opposed to propset. I'm happy to >>> go digging, but any insights others have would be useful. >> >> I completely agree with this assessment. I ran into the same kind of >> thing dealing with "translation" ... or "subst" ... depending on which >> file or random function you happened to look at. Fall 2008, I narrowed >> a lot of this down, but it still remains a bit broader of an API than >> I would prefer. ... and setting file attributes is seemingly falling >> into the same category. When working on that stuff, I certainly recall >> finding stuff that would set those flags scattered around, under >> different names, each for a specialized use. >> >> If you can rationalize them, then kudos. Would love to see it. >> >> Bert mentioned changing perms inadvertedly. Not sure that I read and >> grok'd that entirely. But I do tend to agree with one of your >> assessments: the file should always be sync'd with the "topmost" >> props. Get that done first, and then worry about perf later. And >> regarding perf: meh. I tend to believe the changes in the props >> driving these things is *minimal*, so perf is going to be a moot point >> in the typical case. Correctness, code understandability, and >> determinism is the most important goal. > > I think I've got all the special cases and concerns isolated to a > single function now. There are certainly a few performance > improvements which could be made (grabbing all the data from the db in > one go, rather than several queries), but the logic is all in the > right spot, including a goofy special case for lock test 40. > > Bert's concern, as I understand it, is that before there were code > paths in which we didn't touch the perms at all, but now we > unconditionally clobber whatever perms were there with our own. I'm > fine with that, actually, but I'm not sure what the performance > implications are (I'd presume that OS caches would make the > performance negligible, even on network filesystems).
I'm also fine with your approach. If/when perf problems are actually found, then we can analyze the fix. Additional flags or functions, *with a clear reason for their existence*, would be a fine solution in my mind. I see the problem (before your changes) as having N functions without any true clarity for why they exist. Deviating from your One True Function with clear explanation should avoid going back to that state. Cheers, -g