On Fri, Mar 11, 2022 at 11:18:38AM -0500, Cleber Rosa wrote:
>
> 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).
You are right, makes sense. I will fix with a v2.
Thanks
Beraldo
