Merge lp:~vila/bzr/controversial into lp:bzr

This is the complementary fixes to https://code.edge.launchpad.net/~vila/bzr/freebsd-regressions/+merge/11291
to have a full test passing on FreeBSD.

I'd like some smart server expert advice there for the TestCmdServeChrooting.test_serve_tcp.

 review needs-info

I don't want to merge this as-is, but I have a guess about what's going on...

[...]
> I'd like some smart server expert advice there for the TestCmdServeChrooting.test_serve_tcp.
[...]
> + if sys.platform.startswith('freebsd'):
> + # Something fishy is going on there, the server is able to publish
> + # the port it is listening too, yet the client can't connect...
> + # The assertion below should fail when the problem is fixed or the
> + # test rewritten.
> + self.assertRaises(AttributeError, getattr, self, 'client_resp')

That is odd. By this point bind and listen have been called, which I would have
thought would be enough.

Perhaps it's an IPv6 vs. IPv4 issue? Currently bzr doesn't specify the address
family, it passes AF_UNSPEC to getaddrinfo and uses the address family (and
socktype and proto and address) returned from that. And it'll use
bzrlib.smart.medium.BZR_DEFAULT_INTERFACE (i.e. None) as the host for
getaddrinfo, hmm...

Does the test work if you change it to pass "--port=127.0.0.1:0"? The test
assumes 127.0.0.1 later, so it's reasonable to explicitly pass it here.

If not, does 'netstat -l' (or the equivalent?) report when you do "bzr serve --port
0"?

[...]
> === modified file 'bzrlib/tests/test_smart_transport.py'
[...]
> - self.assertEqual(expected_response, client_sock.recv(50),
> + self.assertEqual(expected_response, osutils.recv_all(client_sock, 50),

This change isn't controversial at all :)

-Andrew.

On Tue, Sep 08, 2009 at 12:00:21AM -0000 I heard the voice of
Andrew Bennetts, and lo! it spake thus:
>
> Perhaps it's an IPv6 vs. IPv4 issue?

Reasonable:

% bzr serve --port 0
listening on port: 39423

% sockstat -l | grep python
fullermd python 65897 4 tcp6 *:39423 *:*
                             ^^^^

--
Matthew Fuller (MF4839) | <email address hidden>
Systems/Network Administrator | http://www.over-yonder.net/~fullermd/
           On the Internet, nobody can hear you scream.

> review needs-info
>
> I don't want to merge this as-is, but I have a guess about what's going on...

> Perhaps it's an IPv6 vs. IPv4 issue?

Exactly (and thanks to fullermd for confirming :-)

> Does the test work if you change it to pass "--port=127.0.0.1:0"?

Perfectly.

> The test
> assumes 127.0.0.1 later, so it's reasonable to explicitly pass it here.

Done.

> This change isn't controversial at all :)

Not in this incarnation :-)

It was discussed with Robert *before* submission and modified *during* submission,
so it ends up trivial instead of controversial :)

The whole patch is now:

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py 2009-08-27 22:17:35 +0000
+++ bzrlib/tests/blackbox/test_serve.py 2009-09-08 03:31:02 +0000
@@ -255,14 +255,15 @@
         t = self.get_transport()
         t.mkdir('server-root')
         self.run_bzr_serve_then_func(
- ['--port', '0', '--directory', t.local_abspath('server-root'),
+ ['--port', '127.0.0.1:0',
+ '--directory', t.local_abspath('server-root'),
              '--allow-writes'],
             self.when_server_started)
         # The when_server_started method issued a find_repositoryV3 that should
         # fail with 'norepository' because there are no repositories inside the
         # --directory.
         self.assertEqual(('norepository',), self.client_resp)
-
+
     def run_bzr_serve_then_func(self, serve_args, func, *func_args,
             **func_kwargs):
         """Run 'bzr serve', and run the given func in a thread once the server

=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py 2009-07-08 07:03:38 +0000
+++ bzrlib/tests/test_smart_transport.py 2009-09-07 09:03:41 +0000
@@ -732,7 +732,7 @@
         client_sock.sendall(rest_of_request_bytes)
         server._serve_one_request(server_protocol)
         server_sock.close()
- self.assertEqual(expected_response, client_sock.recv(50),
+ self.assertEqual(expected_response, osutils.recv_all(client_sock, 50),
                          "Not a version 2 response to 'hello' request.")
         self.assertEqual('', client_sock.recv(1))

Is that ok with you ?

> Is that ok with you ?

Yep. It's nice to guess correctly about a portability issue, for once :)