On Wed, Apr 22, 2020 at 02:18:15PM -0700, Brandon Casey wrote: > > I know that wasn't the original intent of the flag, but I think it was a > > conscious decision to build on around the time of e2d90fd1c, when we > > started actually checking fopen() return values (as opposed to just > > segfaulting). > > Our policy has always been to check return values hasn't it? Policy perhaps, but practice didn't always follow it. :) At any rate, my point was just that as part of a larger cleanup, we found it useful at that time to have an fopen() which behaved predictably on all platforms. > > And in practice, do we really care about cases that can fopen a > > directory but refuse to read from it? It's simpler and more efficient to > > catch this case up front. > > I'm not sure I agree that it's simpler and more efficient to catch > this up front. Catching the case where a directory is supplied when > only a file is valid is an error path which we generally do not > optimize for, and it requires us to add an extra stat to every fopen() > call. We should have always been checking both the fopen() and any > reads and handle a failure in either one. So normally we wouldn't have > to do anything special to produce an error for the case of a directory > being supplied. True, I guess it's not really more efficient (I was thinking that we had to deal with it only once, not on each fread). I do think it's simpler, though, as it means fopen() behaves the same everywhere. > Now, if you want to ignore when a directory is supplied, and not > produce an error, I would expect the code to actually check for that > explicitly/clearly and not depend on fopen() failing, since that is > not a common behavior. Sure, we could be stat()-ing each file before fopen(). And if you think it's worth going back to that direction, you can try to work up a patch. My suspicion is that it will end up making the callers worse, for no real gain. > We've generally taken the approach that there is an expected "normal" > behavior for the c library, generally the linux/glibc behavior. Then, > for platforms that behaved differently from what we've defined as > "normal", we'd introduce a compatibility function to make them behave > the way we wanted them to behave, or as close to that as possible. But > what you're suggesting here seems different. You're suggesting that we > should modify the behavior of fopen from what is commonly considered > "normal" so that it behaves in a new uncommon way. That doesn't seem > like the right thing to do. I think this is just defining a different "normal". And FWIW, I'm not suggesting any particular change now. I think we went in this direction a few years ago, and I'm just trying to explain the current state in terms of that decision. > Instead, I would think it would be better to introduce a new function > that has the behavior we want and to explicitly call that function > instead of pretending that we're calling fopen(). Otherwise it just > leads to confusion since _our_ fopen() doesn't actually work the way > fopen() "normally" works on our common platform. Maybe call it > fopen_file_only() or something. I've been away from git development > for too long to know whether most fopen callsites would need to use an > fopen_file_only() function or whether it would just be a few special > instances, and the rest could just use a regular fopen(). Yeah, I think that would be fine. Though again, I'm really not sure that it gains us all that much, and it would require auditing each fopen() site to see whether it's depending on the current compat behavior or not. -Peff
