Correct undefined name error in LazyFqdn.__str__ #179

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@logic
logic commented Jun 25, 2013

If AddressFamily is set to anything but "any" in your ssh config, this (presumably little-used; it's a fluke of history that we have that enabled in our environment) code path runs, and the undefined "host" variable gets referenced. I'm guessing, at some point in the past, host used to be set to socket.gethostname(); here's a quick patch. :)

@logic logic Correct undefined name error in LazyFqdn.__str__
If AddressFamily is set to anything but "any" in your ssh config, this (presumably little-used; it's a fluke of history that we have that enabled in our environment) code path runs, and the undefined "host" variable gets referenced. I'm guessing, at some point in the past, host used to be set to socket.gethostname(); here's a quick patch. :)
b14f3a4
@karenc
karenc commented Aug 7, 2013

@stefanha @logic Here's the original patch:

fairview@0ed3734

The commits were from #110 and #128. The "host" line was probably removed because of a conflict resolution:

host = socket.gethostname().split('.')[0]

@Kami
Kami commented Aug 21, 2013

(re-posting this from #201)

@stefanha Replacing it with self.config['hostname'] will work (in a way that exception is not thrown), but it's logically incorrect. LazyFqdn is used to replace %l variable which refers to a local host name, but self.config['hostname'] refers to a remote / target host name (%h).

@stefanha

@Kami agreed and thanks for fixing it!

@Kami
Kami commented Aug 21, 2013

@stefanha No problem & thanks :)

@bitprophet
Member

Hi all, sorry for letting this sit around. I checked and it looks like the line utilizing socket.gethostname didn't go away, it was simply not handed to LazyFqdn which encapsulates the line which is breaking. I'm testing a fix now that simply passes in that host value to the constructor :)

As soon as I can convince a regression test to fail correctly I'll merge my fix.

@bitprophet
Member

Welp, got the test failing, but it exposes another issue - socket.getaddrinfo() is likely to fail with socket.gaierror on workstations and any other node where the result of socket.gethostname() isn't valid in local DNS. This may or may not merit a fatal exception in regular usage (guessing not) but it definitely tanks the test for me :)

However in these situations socket.getfqdn() usually still functions, and that's what is used for AddressFamily 'any' or for unknown values. So I think I'll update the codebase to capture socket.gaierror and fallback to the same spot.

@bitprophet bitprophet added a commit that referenced this pull request Sep 27, 2013
@bitprophet bitprophet Changelog re #179 8b983d9
@logic logic deleted the unknown repository branch Oct 1, 2013
@lndbrg lndbrg referenced this pull request Aug 14, 2014
Merged

Window and packet fixes #372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment