Beraldo Leal <bl...@redhat.com> writes:
> On Fri, Mar 11, 2022 at 09:28:24AM -0500, Cleber Rosa wrote: >> >> 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. > > Since this is a public method and could be called anytime before > set_up_cloudinit(), I decided to keep the check just for safety reasons. > Ideally, I would prefer not to have this dependency and add a new > argument, but I didn't want to change the method signature since it > would be required. > I'm not sure I follow your point. Let me try to rephrase mine, in case I failed to communicate it: I can't see how "if not self.phone_server" is a valid check given that it will either: * Contain an instance with a port that is already allocated, OR * Not get assigned if cloudinit.PhoneHomeServer() fails (and raises an exception). Instead of this check, it'd make sense to have a try/except block protecting the PhoneHomeServer instantiation, and canceling the test if it fails. Or maybe you meant to check for self.phone_server.server_port instead? Cheers, - Cleber.