Opened 2 years ago

Last modified 2 weeks ago

#5723 defect new

LogFile.rotate is overly careful in its LBYL code

Reported by: exarkun Owned by: habnabit
Priority: normal Milestone:
Component: core Keywords: review
Cc: Branch:
Author: Launchpad Bug:

Description

       if not (os.access(self.directory, os.W_OK) and os.access(self.path, os.W_OK)):
            return

Write access to a file is not necessary to rename that file. The second part of this check is superfluous and sometimes counterproductive (as the process may have write permission to the directory but not the file). It should be removed.

Additionally, look-before-you-leap is a bad way to write software. Instead, the code should probably just try rotating the file. If it fails with a permission error, that can be silenced.

This may have some relationship with #4707 as well.

Attachments (1)

patch-for-5723.patch (7.9 KB) - added by mriehl 4 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 2 years ago by exarkun

#6073 was a duplicate of this.

Changed 4 months ago by mriehl

comment:2 Changed 4 months ago by mriehl

  • Keywords review added

comment:3 Changed 4 months ago by mriehl

A few notes:

  • IMHO there is a lot of stylistic work left to do on logfile.py (unit test naming conventions not consistent for example) but I wanted to keep this patch as small as possible
  • I fixed some minor docstrings and formatting issues (boyscout)
  • The tests are not unit tests. They perform file I/O - I'd normally use mock for this, but could not find any usage in the codebase. Since all the existing tests also do file I/O, I went with this approach too

comment:4 follow-up: Changed 4 months ago by exarkun

Thanks!

IMHO there is a lot of stylistic work left to do on logfile.py (unit test naming conventions not consistent for example) but I wanted to keep this patch as small as possible

Thank you. This is the preferred approach. :)

The tests are not unit tests. They perform file I/O - I'd normally use mock for this, but could not find any usage in the codebase. Since all the existing tests also do file I/O, I went with this approach too

The local filesystem is almost always available, almost always reliable, it is fast, it is rather deterministic. The benefits of faking it are quite minor whereas the costs of doing so (well) are somewhat high. This isn't to say no one should ever do this but done wrong the solution is worse than the problem and done right this isn't a small undertaking.

comment:5 in reply to: ↑ 4 Changed 4 months ago by mriehl

Replying to exarkun:

The local filesystem is almost always available, almost always reliable, it is fast, it is rather deterministic. The benefits of faking it are quite minor whereas the costs of doing so (well) are somewhat high. This isn't to say no one should ever do this but done wrong the solution is worse than the problem and done right this isn't a small undertaking.

I understand. Another argument would be cross-platform since when using mock you're only micking behaviour and would probably miss some quirks, e.G. on windows.

comment:6 Changed 2 weeks ago by habnabit

  • Owner set to habnabit
Note: See TracTickets for help on using tickets.