Patchwork zeroconf: fail nicely on IPv6 only system

login
register
mail settings
Submitter Simon Farnsworth
Date Feb. 8, 2017, 4:10 p.m.
Message ID <a847eb00fcfeffce458e.1486570248@devvm022.lla2.facebook.com>
Download mbox | patch
Permalink /patch/18350/
State Accepted
Headers show

Comments

Simon Farnsworth - Feb. 8, 2017, 4:10 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1486570121 28800
#      Wed Feb 08 08:08:41 2017 -0800
# Node ID a847eb00fcfeffce458e11b80ad38d4d4e7a700f
# Parent  d50cda2a403786836d1f0d5c99401599dc4f43ec
zeroconf: fail nicely on IPv6 only system

zeroconf only knows how to deal with IPv4; I develop on a system where the only
IPv4 address is 127.0.0.1.

Teach zeroconf to ignore IPv6 addresses when looking for plausible IPv4
connectivity.
Jun Wu - Feb. 9, 2017, 4:24 a.m.
This does not work if the hardcoded 127.0.0.1 is not known to the system:

    sudo ip addr del 127.0.0.1/8 dev lo # on Linux

That could happen if the system is IPv6-only, or only has other addresses in
the 127./8 subnet like 127.0.0.2, or does not have a "lo" device. I think
the pathological environments are not that uncommon with all kinds of
container technologies.

Maybe a better fix is to just skip the test when we detect "dumbip" is not
an IPv4 address.

Excerpts from Simon Farnsworth's message of 2017-02-08 08:10:48 -0800:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486570121 28800
> #      Wed Feb 08 08:08:41 2017 -0800
> # Node ID a847eb00fcfeffce458e11b80ad38d4d4e7a700f
> # Parent  d50cda2a403786836d1f0d5c99401599dc4f43ec
> zeroconf: fail nicely on IPv6 only system
> 
> zeroconf only knows how to deal with IPv4; I develop on a system where the only
> IPv4 address is 127.0.0.1.
> 
> Teach zeroconf to ignore IPv6 addresses when looking for plausible IPv4
> connectivity.
> 
> diff --git a/hgext/zeroconf/__init__.py b/hgext/zeroconf/__init__.py
> --- a/hgext/zeroconf/__init__.py
> +++ b/hgext/zeroconf/__init__.py
> @@ -64,7 +64,9 @@
>      # Generic method, sometimes gives useless results
>      try:
>          dumbip = socket.gethostbyaddr(socket.gethostname())[2][0]
> -        if not dumbip.startswith('127.') and ':' not in dumbip:
> +        if ':' in dumbip:
> +            dumbip = '127.0.0.1'
> +        if not dumbip.startswith('127.'):
>              return dumbip
>      except (socket.gaierror, socket.herror):
>          dumbip = '127.0.0.1'
Simon Farnsworth - Feb. 9, 2017, 11:33 a.m.
There's one chunk in test-paths.t that fails without this change on my 
Facebook IPv6-only devserver, and succeeds with it. Beyond that, I have 
no interest in this code at all.

I chose 127.0.0.1 as the replacement where the "external" address is 
IPv6 because this code already uses 127.0.0.1 as an "error" indicator 
(the last line of context in the patch below). I have zero interest in 
fixing this code beyond making it not crash when test-paths.t invokes it 
for me, so that I can stop doing `./run-tests.py -j50 -l ; hg email -r 
REVS` (relying on me spotting that there have been "unexpected" test 
failures, which has a tendency to go wrong towards the end of the 
working day) and start doing `./run-tests.py -j50 -l && hg email -r 
REVS` (relying on the machine not even invoking `hg email` if there are 
test failures).

If someone else does the work to replace the use of 127.0.0.1 in the 
existing error case with a different value, I'll happily redo this patch 
to follow their lead, but I've already spent more time on this than I'm 
willing to justify for an extension I've never used.

Simon

On 09/02/2017 04:24, Jun Wu wrote:
> This does not work if the hardcoded 127.0.0.1 is not known to the system:
>
>     sudo ip addr del 127.0.0.1/8 dev lo # on Linux
>
> That could happen if the system is IPv6-only, or only has other addresses in
> the 127./8 subnet like 127.0.0.2, or does not have a "lo" device. I think
> the pathological environments are not that uncommon with all kinds of
> container technologies.
>
> Maybe a better fix is to just skip the test when we detect "dumbip" is not
> an IPv4 address.
>
> Excerpts from Simon Farnsworth's message of 2017-02-08 08:10:48 -0800:
>> # HG changeset patch
>> # User Simon Farnsworth <simonfar@fb.com>
>> # Date 1486570121 28800
>> #      Wed Feb 08 08:08:41 2017 -0800
>> # Node ID a847eb00fcfeffce458e11b80ad38d4d4e7a700f
>> # Parent  d50cda2a403786836d1f0d5c99401599dc4f43ec
>> zeroconf: fail nicely on IPv6 only system
>>
>> zeroconf only knows how to deal with IPv4; I develop on a system where the only
>> IPv4 address is 127.0.0.1.
>>
>> Teach zeroconf to ignore IPv6 addresses when looking for plausible IPv4
>> connectivity.
>>
>> diff --git a/hgext/zeroconf/__init__.py b/hgext/zeroconf/__init__.py
>> --- a/hgext/zeroconf/__init__.py
>> +++ b/hgext/zeroconf/__init__.py
>> @@ -64,7 +64,9 @@
>>      # Generic method, sometimes gives useless results
>>      try:
>>          dumbip = socket.gethostbyaddr(socket.gethostname())[2][0]
>> -        if not dumbip.startswith('127.') and ':' not in dumbip:
>> +        if ':' in dumbip:
>> +            dumbip = '127.0.0.1'
>> +        if not dumbip.startswith('127.'):
>>              return dumbip
>>      except (socket.gaierror, socket.herror):
>>          dumbip = '127.0.0.1'
Jun Wu - Feb. 9, 2017, 1:52 p.m.
After reading the surrounding code, I believe 127.0.0.1 is fine. So this
patch looks good to me. Thanks!

The test failure where 127.0.0.1 does not exist is actually caused by other
code. I'll send a fix soon.

Excerpts from Simon Farnsworth's message of 2017-02-09 11:33:07 +0000:
> There's one chunk in test-paths.t that fails without this change on my 
> Facebook IPv6-only devserver, and succeeds with it. Beyond that, I have 
> no interest in this code at all.
> 
> I chose 127.0.0.1 as the replacement where the "external" address is 
> IPv6 because this code already uses 127.0.0.1 as an "error" indicator 
> (the last line of context in the patch below). I have zero interest in 
> fixing this code beyond making it not crash when test-paths.t invokes it 
> for me, so that I can stop doing `./run-tests.py -j50 -l ; hg email -r 
> REVS` (relying on me spotting that there have been "unexpected" test 
> failures, which has a tendency to go wrong towards the end of the 
> working day) and start doing `./run-tests.py -j50 -l && hg email -r 
> REVS` (relying on the machine not even invoking `hg email` if there are 
> test failures).
> 
> If someone else does the work to replace the use of 127.0.0.1 in the 
> existing error case with a different value, I'll happily redo this patch 
> to follow their lead, but I've already spent more time on this than I'm 
> willing to justify for an extension I've never used.
> 
> Simon
> 
> On 09/02/2017 04:24, Jun Wu wrote:
> > This does not work if the hardcoded 127.0.0.1 is not known to the system:
> >
> >     sudo ip addr del 127.0.0.1/8 dev lo # on Linux
> >
> > That could happen if the system is IPv6-only, or only has other addresses in
> > the 127./8 subnet like 127.0.0.2, or does not have a "lo" device. I think
> > the pathological environments are not that uncommon with all kinds of
> > container technologies.
> >
> > Maybe a better fix is to just skip the test when we detect "dumbip" is not
> > an IPv4 address.
> >
> > Excerpts from Simon Farnsworth's message of 2017-02-08 08:10:48 -0800:
> >> # HG changeset patch
> >> # User Simon Farnsworth <simonfar@fb.com>
> >> # Date 1486570121 28800
> >> #      Wed Feb 08 08:08:41 2017 -0800
> >> # Node ID a847eb00fcfeffce458e11b80ad38d4d4e7a700f
> >> # Parent  d50cda2a403786836d1f0d5c99401599dc4f43ec
> >> zeroconf: fail nicely on IPv6 only system
> >>
> >> zeroconf only knows how to deal with IPv4; I develop on a system where the only
> >> IPv4 address is 127.0.0.1.
> >>
> >> Teach zeroconf to ignore IPv6 addresses when looking for plausible IPv4
> >> connectivity.
> >>
> >> diff --git a/hgext/zeroconf/__init__.py b/hgext/zeroconf/__init__.py
> >> --- a/hgext/zeroconf/__init__.py
> >> +++ b/hgext/zeroconf/__init__.py
> >> @@ -64,7 +64,9 @@
> >>      # Generic method, sometimes gives useless results
> >>      try:
> >>          dumbip = socket.gethostbyaddr(socket.gethostname())[2][0]
> >> -        if not dumbip.startswith('127.') and ':' not in dumbip:
> >> +        if ':' in dumbip:
> >> +            dumbip = '127.0.0.1'
> >> +        if not dumbip.startswith('127.'):
> >>              return dumbip
> >>      except (socket.gaierror, socket.herror):
> >>          dumbip = '127.0.0.1'
>
Augie Fackler - Feb. 10, 2017, 2:50 a.m.
On Wed, Feb 08, 2017 at 08:10:48AM -0800, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486570121 28800
> #      Wed Feb 08 08:08:41 2017 -0800
> # Node ID a847eb00fcfeffce458e11b80ad38d4d4e7a700f
> # Parent  d50cda2a403786836d1f0d5c99401599dc4f43ec
> zeroconf: fail nicely on IPv6 only system

Queued, thanks.

> 
> zeroconf only knows how to deal with IPv4; I develop on a system where the only
> IPv4 address is 127.0.0.1.
> 
> Teach zeroconf to ignore IPv6 addresses when looking for plausible IPv4
> connectivity.
> 
> diff --git a/hgext/zeroconf/__init__.py b/hgext/zeroconf/__init__.py
> --- a/hgext/zeroconf/__init__.py
> +++ b/hgext/zeroconf/__init__.py
> @@ -64,7 +64,9 @@
>     # Generic method, sometimes gives useless results
>     try:
>         dumbip = socket.gethostbyaddr(socket.gethostname())[2][0]
> -        if not dumbip.startswith('127.') and ':' not in dumbip:
> +        if ':' in dumbip:
> +            dumbip = '127.0.0.1'
> +        if not dumbip.startswith('127.'):
>             return dumbip
>     except (socket.gaierror, socket.herror):
>         dumbip = '127.0.0.1'
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/zeroconf/__init__.py b/hgext/zeroconf/__init__.py
--- a/hgext/zeroconf/__init__.py
+++ b/hgext/zeroconf/__init__.py
@@ -64,7 +64,9 @@ 
     # Generic method, sometimes gives useless results
     try:
         dumbip = socket.gethostbyaddr(socket.gethostname())[2][0]
-        if not dumbip.startswith('127.') and ':' not in dumbip:
+        if ':' in dumbip:
+            dumbip = '127.0.0.1'
+        if not dumbip.startswith('127.'):
             return dumbip
     except (socket.gaierror, socket.herror):
         dumbip = '127.0.0.1'