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

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

On 2/3/21 12: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.
> 

qemu.utils, with the s!

And, OK, this makes me feel less weird about creating a qemu.utils 
package, actually.

ACK (but not reviewed, tested or linted) for the new code heading into 
python/qemu/.

--js

> 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
> 
> 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

We can probably delete the :rtype: field, can't we? Annotations should 
handle this information now.

(Unless some tool needs it for compatibility reasons, but we don't use 
this field anywhere else in ./python/ yet.)

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.

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

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

>
> 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

>           self.log.debug("sshd listening on port:" + port)
> -        return port
> -
> -    def ssh_connect(self, username, password):
> -        self.ssh_logger = logging.getLogger('ssh')
> -        port = self.get_portfwd()
>           self.ssh_session = ssh.Session(self.VM_IP, port=int(port),
>                                          user=username, password=password)
>           for i in range(10):
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 0f9d7a5d9c..bf99164fcb 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -10,6 +10,7 @@ from avocado_qemu import wait_for_console_pattern
>   from avocado.utils import ssh
>   
>   from qemu.accel import kvm_available
> +from qemu.utils import get_info_usernet_hostfwd_port
>   
>   from boot_linux import BootLinux
>   
> @@ -76,27 +77,14 @@ class VirtiofsSubmountsTest(BootLinux):
>       :avocado: tags=arch:x86_64
>       """
>   
> -    def get_portfwd(self):
> -        port = None
> -
> +    def ssh_connect(self, username, keyfile):
> +        self.ssh_logger = logging.getLogger('ssh')
>           res = self.vm.command('human-monitor-command',
>                                 command_line='info usernet')
> -        for line in res.split('\r\n'):
> -            match = \
> -                re.search(r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.',
> -                          line)
> -            if match is not None:
> -                port = int(match[1])
> -                break
> -
> +        port = get_info_usernet_hostfwd_port(res)
>           self.assertIsNotNone(port)
>           self.assertGreater(port, 0)
>           self.log.debug('sshd listening on port: %d', port)
> -        return port
> -
> -    def ssh_connect(self, username, keyfile):
> -        self.ssh_logger = logging.getLogger('ssh')
> -        port = self.get_portfwd()
>           self.ssh_session = ssh.Session('127.0.0.1', port=port,
>                                          user=username, key=keyfile)
>           for i in range(10):
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 00f1d5ca8d..75ce07df36 100644
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -21,6 +21,7 @@ import datetime
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>   from qemu.accel import kvm_available
>   from qemu.machine import QEMUMachine
> +from qemu.utils import get_info_usernet_hostfwd_port
>   import subprocess
>   import hashlib
>   import argparse
> @@ -306,11 +307,7 @@ class BaseVM(object):
>           self.console_init()
>           usernet_info = guest.qmp("human-monitor-command",
>                                    command_line="info usernet")
> -        self.ssh_port = None
> -        for l in usernet_info["return"].splitlines():
> -            fields = l.split()
> -            if "TCP[HOST_FORWARD]" in fields and "22" in fields:
> -                self.ssh_port = l.split()[3]
> +        self.ssh_port = get_info_usernet_hostfwd_port(usernet_info)
>           if not self.ssh_port:
>               raise Exception("Cannot find ssh port from 'info usernet':\n%s" % \
>                               usernet_info)

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.

On 2/15/21 1:27 PM, Cleber Rosa wrote:
> 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.
> 

Yeah. We have a "qmp" package and a "machine" package, and these seem 
very well-defined.

Then we have everything else which is mostly a few random bits and 
pieces (at the moment: just accel.py). Over time those bits and pieces 
might take shape as something more important/meaningful, but for now 
it's pretty nebulous.

An emerging pattern I see is that these functions are "environment 
analysis" helpers; things designed to help interrogate the local 
environment to choose appropriate QEMU flags, or otherwise "QEMU output 
analysis", things designed to make better sense of the output received 
from QEMU.

accel.py is the former, this patch targets the latter.

I suspect accel.py will want to belong to "smart" tools for booting up 
an arbitrary VM (interrogate, decide on config, launch) whereas this 
patch fits into a class of API-esque tools designed for making sense of 
I/O information.

It's meant to digest HMP, though -- is this evidence that we need a 
better QMP command? We shouldn't be using HMP for machine driven 
testing. (I know we have to sometimes, but we should be working towards 
eliminating it.)

-js

On 2/15/21 3:27 PM, Cleber Rosa wrote:
> 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).

I confess I was thinking on the impact of many small/simple tests on CI 
if that become a trend. Time to run a job should be manageable. In this 
case, having unittests for the utilities is all we need and would save 
some minutes on CI.

On the absence of unittests, I'm not opposed to merge this test.

>
>>> 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.
I see. I agree with that design, I will keep it in mind when reviewing 
patches and submitting changes.
>
> Let me know if my explanations make sense and change your mind
> any bit :).

Sure, thanks for the explanations. As I mentioned in my first reply, 
they were just suggestions. So,

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> Thanks for the review!
> - Cleber.

On Thu, Feb 04, 2021 at 07:25:52PM -0500, John Snow wrote:
> On 2/3/21 12: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.
> > 
> 
> qemu.utils, with the s!
>

Nice catch, thanks!

- Cleber.