On Tue, Aug 28, 2018 at 06:08:46PM -0700, Kevin J. McCarthy wrote: > > Though, it seems it leaves in the stat comparison if > > link() succeeds, which is superfluous (could cause needless > > performance hit on slow networked filesystems), and violates KISS and > > "Don't write code you don't need" principles > > My strategy with these two patches was to change the code as little as > possible. I agree the stats if link() succeeds are now 95% useless, but > my thinking was they were there before and provided some kind of check > that the/a target was actually there after the link. I could be > persuaded to pull them inside the #if 0, but I figure they are doing as > much harm as they have been for the past 20 years... :-/
I agree with your assessment, except for one detail: The check is next to useless because we already know the link succeeded, and after the fact the check has a race condition: Even if the check succeeds, you can't know that the file will still exist when Mutt next tries to do something to it. So it really doesn't do anything useful. In retrospect it seems clear to me that whoever wrote that code just got it wrong. The harm is minimal, except that as I pointed out, it can cause a performance hit. On a networked file system, stat() is a relatively expensive operation (in the sense that the data transfer is predominated by overhead--round trip time and network headers). If the network is slow, then the more of these your application does, the worse it will perform. So for example, this could have a significant performance impact on maildir over NFS, if, say, you tag a bunch of messages in order to mark them as not new. On a fast, unloaded network, the user is unlikely to notice at all. If the network is just slow, the user may or may not notice, depending on how many messages... But if the network is congested, you're only adding to the congestion by doing unnecessary operations. It would be interesting to do a large test, comparing the time with and without those stats, ideally on a slow network... So, I think the post-success stat() calls should should come out, and I wouldn't bother using #if to remove it. I think it's clearly just a bad idea to do that check there, and we should remove any trace of it so that no one is tempted to put it back in at some point in the future. But if you feel there's a reason to keep it I won't continue to argue the point. > > It obviously also does not attempt to address the follow symlink > > behavior I mentioned, which is also fine. It may well be that > > safe_rename() is never used in contexts where that's likely to matter, > > so it's possible that it would be reasonable to ignore that issue. > > Yes, my thinking was like that. Technically I think you have a valid > point, but the code apparently wasn't used in any context where the > source was a symlink. It would have failed the compare_stat() in that > case and gone into an infinite loop too, if it were. Sure. It's easy enough to address if a user hits it--probably worth documenting (in the code) though, if nothing else. -- Derek D. Martin http://www.pizzashack.org/ GPG Key ID: 0xDFBEAD02 -=-=-=-=- This message is posted from an invalid address. Replying to it will result in undeliverable mail due to spam prevention. Sorry for the inconvenience.
pgp9ctYkAUAeg.pgp
Description: PGP signature
