Opened 7 years ago

Last modified 7 years ago

#5529 defect new

DNS client does not use correct hosts file no Windows

Reported by: Jared Owned by: Jared
Priority: normal Milestone:
Component: names Keywords: dns names
Cc: Thijs Triemstra, Richard Wall Branch:
Author:

Description

The hosts file on Windoze is in C:\Windows\System32\drivers\etc\hosts, not C:\Windows\hosts.

Furthermore, the %SYSTEMROOT% environment variable should be used, because there is no requirement for Windows to be installed on the 'C:' drive

Lastly, the block to configure the resolver for Windows should not operate for non-posix platforms but specifically only on Windows platforms.

Attachments (3)

dns-resolver-windows-hosts.patch (1.1 KB) - added by Jared 7 years ago.
Use correct hosts file, detect Windows properly, and use system-specific "Windows' folder
dns-resolver-windows-hosts.2.patch (3.5 KB) - added by Jared 7 years ago.
Moves host file location detection to the hosts.Resolver class and provides a tests case
dns-resolver-windows-hosts.3.patch (3.4 KB) - added by Jared 7 years ago.
Implements 1-3 of the request changes to the previous patch

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by Jared

Use correct hosts file, detect Windows properly, and use system-specific "Windows' folder

comment:1 Changed 7 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to Jared

Thanks for your patch. This will need unit tests before it can be accepted into trunk. Btw twisted.python.win32.getProgramsMenuPath and getProgramFilesPath use registry magic to detect the paths, is this still correct, and/or can it be simplified?

comment:2 Changed 7 years ago by Jared

Status: newassigned

%PROGRAMFILES% should contain the location of program files.

Changed 7 years ago by Jared

Moves host file location detection to the hosts.Resolver class and provides a tests case

comment:3 Changed 7 years ago by Jared

Keywords: review added

comment:4 Changed 7 years ago by arsenerei

Thanks for your effort.

  1. In twisted/names/test/test_hosts.py, multi-line imports should use parentheses () and not slashes.
  2. findHostsFile should be private (e.g., _ findHostsFile).
  3. findHostsFile should not have an unhandled case at this point. I'm not aware of any non-Windows and non-POSIX type OS, so you may to just use else instead of elsif.
  4. In twisted/names/test/test_hosts.py, you want to exercise the functionality of findHostsFile, not necessarily the comments inside. There should be a test for the return value of findHostsFile (based on platform) instead.

comment:5 Changed 7 years ago by Jared

  1. In twisted.python.platform.getType() there is a third case: 'java'. Assuming that anything non posix is windows is foolish
  1. I do not see the usefulness for such a test case, as it merely would compare two hash tables, instead of proving that a usable hosts file was found

All this for a one line code change! This is worse than governmental compliance!

Changed 7 years ago by Jared

Implements 1-3 of the request changes to the previous patch

comment:6 Changed 7 years ago by Thijs Triemstra

Owner: Jared deleted
Status: assignednew

comment:7 Changed 7 years ago by David Reid

Keywords: review removed
Owner: set to Jared

Hi greezybacon, thank you a lot for sticking with the process, I know it can be sometimes frustrating but the process works.

A couple more quick points.

  1. In twisted/names/hosts.py in Resolver.init
            if file is None: file = _findHostsFile() 
    

should be

        if file is None:
            file = _findHostsFile() 
  1. Keyword arguments shouldn't have spaces around the '=' since your changing the line it might be nice to bring the ttl argument up to date with that requirement.
  2. Testing _findHostsFile
    1. Write a test for windows and a test for non-windows. You don't want a developer who doesn't use windows to be able to break the windows behavior and not notice because the test passes on his non-windows system.
      1. Use http://twistedmatrix.com/documents/11.0.0/api/twisted.trial.unittest.TestCase.html#patch to monkeypatch platform.isWindows and os.environ
      2. Assert that the expected location is returned based on the os.environ you've patched in.

comment:8 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra Richard Wall added
Note: See TracTickets for help on using tickets.