Re: [PATCH 10/22] Python: add utility function for retrieving port redirection

From: Cleber Rosa <crosa@redhat.com>
To: Wainer dos Santos Moschetta <wainersm@redhat.com>
Cc: "Fam Zheng" <fam@euphon.net>,
	"Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
	"Beraldo Leal" <bleal@redhat.com>, "John Snow" <jsnow@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	qemu-devel@nongnu.org, "Eric Auger" <eauger@redhat.com>,
	"Willian Rampazzo" <wrampazz@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Max Reitz" <mreitz@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Eduardo Habkost" <ehabkost@redhat.com>
Subject: Re: [PATCH 10/22] Python: add utility function for retrieving port redirection
Date: Mon, 15 Feb 2021 13:27:14 -0500	[thread overview]
Message-ID: <20210215182714.GC72984@localhost.localdomain> (raw)
In-Reply-To: <4d848476-c5b4-2fd0-cbcc-01f87e4dfb71@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 7921 bytes --]

On Tue, Feb 09, 2021 at 11:50:51AM -0300, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 2/3/21 2:23 PM, Cleber Rosa wrote:
> > Slightly different versions for the same utility code are currently
> > present on different locations.  This unifies them all, giving
> > preference to the version from virtiofs_submounts.py, because of the
> > last tweaks added to it.
> > 
> > While at it, this adds a "qemu.util" module to host the utility
> > function and a test.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   python/qemu/utils.py                     | 35 ++++++++++++++++++++++++
> >   tests/acceptance/info_usernet.py         | 29 ++++++++++++++++++++
> >   tests/acceptance/linux_ssh_mips_malta.py | 16 +++++------
> >   tests/acceptance/virtiofs_submounts.py   | 20 +++-----------
> >   tests/vm/basevm.py                       |  7 ++---
> >   5 files changed, 77 insertions(+), 30 deletions(-)
> >   create mode 100644 python/qemu/utils.py
> >   create mode 100644 tests/acceptance/info_usernet.py
> 
> I've a few suggestions:
> 
> - IMHO "utils" is too broad, people may start throwing random stuffs there.
> Maybe call it "net". I am sure there will be more net-related code to be
> clustered on that module in the future.
>

It's hard to find the right balance here.  If you take a look at what John
is proposing wrt the packaging the "qemu" Python libs, I believe one module
is a good compromise at this point.  I really to expect that it will grow
and that more modules will be created.

> - Rename the method to "get_hostfwd_port()" because the parameter's name
> already implies it is expected an "info usernet" output.
>

I thought about the method name, and decided to keep the more verbose name
because this method is *not* about retrieving the "hostfwd port" from a
running QEMU, but rather from the output that should be produced by a "info
usernet" command.  It may become the foundation of a method on the QEMUMachine
class that is called "get_hostfwd_port()" as you suggested, that includes
getting the data (that is, issuing a "info usernet" command).

Anyway, I tend to favor "explicit is better than implicit" approach, but
I recognize that I can be too verbose at times.  Let's see if other people
chip in with naming suggestions.

> - Drop the InfoUsernet Test. It is too simple, and the same functionality is
> tested indirectly by other tests.
>

I find "too simple" is a typical case of "famous last words" :D.
Now, as a functional test to cover the utility function, it's indeed
a bit of overkill.  It'd probably be less necessary if there were unittests
for those (and there will hopefully be some soon).

Now, testing *directly* was indeed the intention here. I see a few
reasons for that, including saving a good amount of debugging time:
such a test failing would provide *direct* indication of where the
regression is.  These simpler tests can also be run with targets other
than the ones really connecting into guests at this moment (while it's
debatable wether such a regression would appear only on such targets).

> > 
> > diff --git a/python/qemu/utils.py b/python/qemu/utils.py
> > new file mode 100644
> > index 0000000000..89a246ab30
> > --- /dev/null
> > +++ b/python/qemu/utils.py
> > @@ -0,0 +1,35 @@
> > +"""
> > +QEMU utility library
> > +
> > +This offers miscellaneous utility functions, which may not be easily
> > +distinguishable or numerous to be in their own module.
> > +"""
> > +
> > +# Copyright (C) 2021 Red Hat Inc.
> > +#
> > +# Authors:
> > +#  Cleber Rosa <crosa@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2.  See
> > +# the COPYING file in the top-level directory.
> > +#
> > +
> > +import re
> > +from typing import Optional
> > +
> > +
> > +def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
> > +    """
> > +    Returns the port given to the hostfwd parameter via info usernet
> > +
> > +    :param info_usernet_output: output generated by hmp command "info usernet"
> > +    :param info_usernet_output: str
> > +    :return: the port number allocated by the hostfwd option
> > +    :rtype: int
> > +    """
> > +    for line in info_usernet_output.split('\r\n'):
> > +        regex = r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.'
> > +        match = re.search(regex, line)
> > +        if match is not None:
> > +            return int(match[1])
> > +    return None
> > diff --git a/tests/acceptance/info_usernet.py b/tests/acceptance/info_usernet.py
> > new file mode 100644
> > index 0000000000..9c1fd903a0
> > --- /dev/null
> > +++ b/tests/acceptance/info_usernet.py
> > @@ -0,0 +1,29 @@
> > +# Test for the hmp command "info usernet"
> > +#
> > +# Copyright (c) 2021 Red Hat, Inc.
> > +#
> > +# Author:
> > +#  Cleber Rosa <crosa@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > +# later.  See the COPYING file in the top-level directory.
> > +
> > +from avocado_qemu import Test
> > +
> > +from qemu.utils import get_info_usernet_hostfwd_port
> > +
> > +
> > +class InfoUsernet(Test):
> > +
> > +    def test_hostfwd(self):
> > +        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22')
> > +        self.vm.launch()
> > +        res = self.vm.command('human-monitor-command',
> > +                              command_line='info usernet')
> > +        port = get_info_usernet_hostfwd_port(res)
> > +        self.assertIsNotNone(port,
> > +                             ('"info usernet" output content does not seem to '
> > +                              'contain the redirected port'))
> > +        self.assertGreater(port, 0,
> > +                           ('Found a redirected port that is not greater than'
> > +                            ' zero'))
> > diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
> > index 25c5c5f741..275659c785 100644
> > --- a/tests/acceptance/linux_ssh_mips_malta.py
> > +++ b/tests/acceptance/linux_ssh_mips_malta.py
> > @@ -18,6 +18,8 @@ from avocado.utils import process
> >   from avocado.utils import archive
> >   from avocado.utils import ssh
> > +from qemu.utils import get_info_usernet_hostfwd_port
> > +
> >   class LinuxSSH(Test):
> > @@ -70,18 +72,14 @@ class LinuxSSH(Test):
> >       def setUp(self):
> >           super(LinuxSSH, self).setUp()
> > -    def get_portfwd(self):
> > +    def ssh_connect(self, username, password):
> > +        self.ssh_logger = logging.getLogger('ssh')
> >           res = self.vm.command('human-monitor-command',
> >                                 command_line='info usernet')
> > -        line = res.split('\r\n')[2]
> > -        port = re.split(r'.*TCP.HOST_FORWARD.*127\.0\.0\.1 (\d+)\s+10\..*',
> > -                        line)[1]
> > +        port = get_info_usernet_hostfwd_port(res)
> > +        if not port:
> > +            self.cancel("Failed to retrieve SSH port")
> 
> Here I think it should assert not none, otherwise there is a bug somewhere.
>
> - Wainer

I'm trying to be careful and conservative with adding assertions,
because IMO those belong to test writers.  IMO the expectation of a
user writing a test using "ssh_connect()" as a framework utility,
would be more aligned with the framework bailing out, than blatantly
setting a test failure.

It's similar to what happens if a "vmimage" snapshot can not be
created... there's an issue somewhere, but it'd be a bit unfair, and
confusing, to set a assertion error (and thus test failure).  But, I
think this is the kind of design decision that will evolve with (more)
time here.

Let me know if my explanations make sense and change your mind
any bit :).

Thanks for the review!
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]