On 21.01.19 21:50, John Snow wrote: > > > On 12/21/18 6:47 PM, Max Reitz wrote: >> To do this, we need to allow creating the NBD server on various ports >> instead of a single one (which may not even work if you run just one >> instance, because something entirely else might be using that port). >> >> So we just pick a random port in [32768, 32768 + 1024) and try to create >> a server there. If that fails, we just retry until something sticks. >> >> For the IPv6 test, we need a different range, though (just above that >> one). This is because "localhost" resolves to both 127.0.0.1 and ::1. >> This means that if you bind to it, it will bind to both, if possible, or >> just one if the other is already in use. Therefore, if the IPv6 test >> has already taken [::1]:some_port and we then try to take >> localhost:some_port, that will work -- only the second server will be >> bound to 127.0.0.1:some_port alone and not [::1]:some_port in addition. >> So we have two different servers on the same port, one for IPv4 and one >> for IPv6. >> >> But when we then try to connect to the server through >> localhost:some_port, we will always end up at the IPv6 one (as long as >> it is up), and this may not be the one we want. >> >> Thus, we must make sure not to create an IPv6-only NBD server on the >> same port as a normal "dual-stack" NBD server -- which is done by using >> distinct port ranges, as explained above. >> >> Signed-off-by: Max Reitz <[email protected]> >> --- >> tests/qemu-iotests/147 | 98 +++++++++++++++++++++++++++++------------- >> 1 file changed, 68 insertions(+), 30 deletions(-) >> >> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147 >> index 3e10a9969e..82513279b0 100755 >> --- a/tests/qemu-iotests/147 >> +++ b/tests/qemu-iotests/147 >> @@ -19,13 +19,17 @@ >> # >> >> import os >> +import random >> import socket >> import stat >> import time >> import iotests >> -from iotests import cachemode, imgfmt, qemu_img, qemu_nbd >> +from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_pipe >> >> -NBD_PORT = 10811 >> +NBD_PORT_START = 32768 >> +NBD_PORT_END = NBD_PORT_START + 1024 >> +NBD_IPV6_PORT_START = NBD_PORT_END >> +NBD_IPV6_PORT_END = NBD_IPV6_PORT_START + 1024 >> >> test_img = os.path.join(iotests.test_dir, 'test.img') >> unix_socket = os.path.join(iotests.test_dir, 'nbd.socket') >> @@ -88,17 +92,29 @@ class QemuNBD(NBDBlockdevAddBase): >> except OSError: >> pass >> >> + def _try_server_up(self, *args): >> + status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args) >> + if status == 0: >> + return True >> + if 'Address already in use' in msg: >> + return False > > My only half-empty question is if we are guaranteed to have a locale and > environment such that this string will always be present when we > encounter that specific error? > > I suppose even if not, that it'll just fail hard and it's no worse than > what we had before.
I can tell you at least that my locale is de_DE.UTF-8.
>> + self.fail(msg)
>> +
>> def _server_up(self, *args):
>> - self.assertEqual(qemu_nbd('-f', imgfmt, test_img, *args), 0)
>> + self.assertTrue(self._try_server_up(*args))
>>
>> def test_inet(self):
>> - self._server_up('-b', 'localhost', '-p', str(NBD_PORT))
>> + while True:
>> + nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>> + if self._try_server_up('-b', 'localhost', '-p', str(nbd_port)):
>> + break
>
> Why not just iterate over the range so that once the range is exhausted
> we're guaranteed to fail?
Hm. With this approach chances are higher that the first port you try
works, so it does have a benefit. I'm not sure whether it's actually a
problem that this will pretty much break down if everything in the range
is allocated, because I can't see that happening.
Iterating over a range wouldn't make the code easier because I would
have to account for the error case. O:-)
>> +
>> address = { 'type': 'inet',
>> 'data': {
>> 'host': 'localhost',
>> - 'port': str(NBD_PORT)
>> + 'port': str(nbd_port)
>> } }
>> - self.client_test('nbd://localhost:%i' % NBD_PORT,
>> + self.client_test('nbd://localhost:%i' % nbd_port,
>> flatten_sock_addr(address))
>>
>> def test_unix(self):
>> @@ -130,8 +146,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
>> except OSError:
>> pass
>>
>> - def _server_up(self, address, export_name=None, export_name2=None):
>> + # Returns False on EADDRINUSE; fails an assertion on other errors.
>> + # Returns True on success.
>> + def _try_server_up(self, address, export_name=None, export_name2=None):
>> result = self.server.qmp('nbd-server-start', addr=address)
>> + if 'error' in result and \
>> + 'Address already in use' in result['error']['desc']:
>> + return False
>> self.assert_qmp(result, 'return', {})
>>
>> if export_name is None:
>> @@ -146,20 +167,28 @@ class BuiltinNBD(NBDBlockdevAddBase):
>> name=export_name2)
>> self.assert_qmp(result, 'return', {})
>>
>> + return True
>> +
>> + def _server_up(self, address, export_name=None, export_name2=None):
>> + self.assertTrue(self._try_server_up(address, export_name,
>> export_name2))
>>
>> def _server_down(self):
>> result = self.server.qmp('nbd-server-stop')
>> self.assert_qmp(result, 'return', {})
>>
>> def do_test_inet(self, export_name=None):
>> - address = { 'type': 'inet',
>> - 'data': {
>> - 'host': 'localhost',
>> - 'port': str(NBD_PORT)
>> - } }
>> - self._server_up(address, export_name)
>> + while True:
>> + nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>> + address = { 'type': 'inet',
>> + 'data': {
>> + 'host': 'localhost',
>> + 'port': str(nbd_port)
>> + } }
>> + if self._try_server_up(address, export_name):
>> + break
>> +
>> export_name = export_name or 'nbd-export'
>> - self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, export_name),
>> + self.client_test('nbd://localhost:%i/%s' % (nbd_port, export_name),
>> flatten_sock_addr(address), export_name)
>> self._server_down()
>>
>> @@ -173,15 +202,19 @@ class BuiltinNBD(NBDBlockdevAddBase):
>> self.do_test_inet('shadow')
>>
>> def test_inet_two_exports(self):
>> - address = { 'type': 'inet',
>> - 'data': {
>> - 'host': 'localhost',
>> - 'port': str(NBD_PORT)
>> - } }
>> - self._server_up(address, 'exp1', 'exp2')
>> - self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp1'),
>> + while True:
>> + nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>> + address = { 'type': 'inet',
>> + 'data': {
>> + 'host': 'localhost',
>> + 'port': str(nbd_port)
>> + } }
>> + if self._try_server_up(address, 'exp1', 'exp2'):
>> + break
>> +
>> + self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp1'),
>> flatten_sock_addr(address), 'exp1', 'node1', False)
>> - self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp2'),
>> + self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp2'),
>> flatten_sock_addr(address), 'exp2', 'node2', False)
>> result = self.vm.qmp('blockdev-del', node_name='node1')
>> self.assert_qmp(result, 'return', {})
>> @@ -197,20 +230,25 @@ class BuiltinNBD(NBDBlockdevAddBase):
>> except socket.gaierror:
>> # IPv6 not available, skip
>> return
>> - address = { 'type': 'inet',
>> - 'data': {
>> - 'host': '::1',
>> - 'port': str(NBD_PORT),
>> - 'ipv4': False,
>> - 'ipv6': True
>> - } }
>> +
>> + while True:
>> + nbd_port = random.randrange(NBD_IPV6_PORT_START,
>> NBD_IPV6_PORT_END)
>> + address = { 'type': 'inet',
>> + 'data': {
>> + 'host': '::1',
>> + 'port': str(nbd_port),
>> + 'ipv4': False,
>> + 'ipv6': True
>> + } }
>> + if self._try_server_up(address):
>> + break
>> +
>> filename = { 'driver': 'raw',
>> 'file': {
>> 'driver': 'nbd',
>> 'export': 'nbd-export',
>> 'server': flatten_sock_addr(address)
>> } }
>> - self._server_up(address)
>> self.client_test(filename, flatten_sock_addr(address), 'nbd-export')
>> self._server_down()
>>
>>
>
> I guess my only other observation is that we have a lot of "while True"
> loops now, is it worth creating some kind of helper that does the dirty
> work of finding a serviceable port or nah?
Seems reasonable, I'll see how it looks.
> If none of my observations spark joy:
>
> 1-3: Reviewed-By: John Snow <[email protected]>
Thanks!
Max
signature.asc
Description: OpenPGP digital signature
