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.
> 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.
Makes sense.
> Other than those points, the direction of those changes are indeed a
> great improvement.
Thank you all, I will also remove the unused 'network' import on a v2,
that I just notice after sending the patch.
--
Beraldo
