Beraldo Leal <bl...@redhat.com> writes:
> Race conditions can happen with the current code, because the port that > was available might not be anymore by the time the server is started. > > By setting the port to 0, PhoneServer it will use the OS default > behavior to get a free port, then we save this information so we can > later configure the guest. > > Suggested-by: Daniel P. Berrangé <berra...@redhat.com> > Signed-off-by: Beraldo Leal <bl...@redhat.com> > --- > tests/avocado/avocado_qemu/__init__.py | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/tests/avocado/avocado_qemu/__init__.py > b/tests/avocado/avocado_qemu/__init__.py > index 9b056b5ce5..e830d04b84 100644 > --- a/tests/avocado/avocado_qemu/__init__.py > +++ b/tests/avocado/avocado_qemu/__init__.py > @@ -602,9 +602,8 @@ def prepare_cloudinit(self, ssh_pubkey=None): > self.log.info('Preparing cloudinit image') > try: > cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso') > - self.phone_home_port = network.find_free_port() > - if not self.phone_home_port: > - self.cancel('Failed to get a free port') > + if not self.phone_server: > + self.cancel('Failed to get port used by the PhoneServer.') Can you think of a condition where `self.phone_server` would not evaluate to True? `network.find_free_port()` could return None, so this check was valid. But now with `cloudinit.PhoneHomeServer`, I can not see how we'd end up with a similar condition. Instantiating `cloudinit.PhoneHomeServer` where a port can not be alloccated, AFAICT, would raise a socket exception instead. Also, the name of the utility class is PhoneHomeServer. Using a different name in the message will make cross references into the Avocado docs harder. Finally, a nitpick: I'd drop the leading dot in such a test cancelation message. Other than those points, the direction of those changes are indeed a great improvement. Thanks, - Cleber.