| Message ID | 20210323221539.3532660-4-crosa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Acceptance Test: introduce base class for Linux based tests | expand |
Hi Cleber, On 3/23/21 11:15 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.utils" module to host the utility > function and a test. > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > Reviewed-by: Wainer dos Santos Moschetta <wainersm@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 | 21 ++++---------- > tests/vm/basevm.py | 7 ++--- > 5 files changed, 78 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 > + """ > + 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 6dbd02d49d..052008f02d 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 archive > from avocado.utils import ssh > > +from qemu.utils import get_info_usernet_hostfwd_port > + > > class LinuxSSH(Test): > > @@ -70,18 +72,14 @@ def get_kernel_info(self, endianess, wordsize): > 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") > 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 ca64b76301..57a7047342 100644 > --- a/tests/acceptance/virtiofs_submounts.py > +++ b/tests/acceptance/virtiofs_submounts.py > @@ -9,6 +9,8 @@ > from avocado_qemu import wait_for_console_pattern > from avocado.utils import ssh > > +from qemu.utils import get_info_usernet_hostfwd_port > + > > def run_cmd(args): > subp = subprocess.Popen(args, > @@ -73,27 +75,14 @@ class VirtiofsSubmountsTest(LinuxTest): > :avocado: tags=accel:kvm > """ > > - 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 @@ > 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 @@ def boot(self, img, extra_args=[]): > 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) > Looks good to me Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
On Tue, Mar 23, 2021 at 7:16 PM Cleber Rosa <crosa@redhat.com> 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.utils" module to host the utility > function and a test. > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > Reviewed-by: Wainer dos Santos Moschetta <wainersm@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 | 21 ++++---------- > tests/vm/basevm.py | 7 ++--- > 5 files changed, 78 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 > + """ > + 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') This pattern is repeated every time a test needs to call get_info_usernet_hostfwd_port. Do you see any downside on moving this line inside the function and passing self.vm as an argument? This would abstract the need to run info usernet before calling the function. > + 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 6dbd02d49d..052008f02d 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 archive > from avocado.utils import ssh > > +from qemu.utils import get_info_usernet_hostfwd_port > + > > class LinuxSSH(Test): > > @@ -70,18 +72,14 @@ def get_kernel_info(self, endianess, wordsize): > 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") > 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 ca64b76301..57a7047342 100644 > --- a/tests/acceptance/virtiofs_submounts.py > +++ b/tests/acceptance/virtiofs_submounts.py > @@ -9,6 +9,8 @@ > from avocado_qemu import wait_for_console_pattern > from avocado.utils import ssh > > +from qemu.utils import get_info_usernet_hostfwd_port > + > > def run_cmd(args): > subp = subprocess.Popen(args, > @@ -73,27 +75,14 @@ class VirtiofsSubmountsTest(LinuxTest): > :avocado: tags=accel:kvm > """ > > - 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 @@ > 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 @@ def boot(self, img, extra_args=[]): > 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) > -- > 2.25.4 > Other than maybe a small adjustment to the get_info_usernet_hostfwd_port function: Reviewed-by: Willian Rampazzo <willianr@redhat.com>
On Wed, Mar 24, 2021 at 05:35:07PM -0300, Willian Rampazzo wrote: > On Tue, Mar 23, 2021 at 7:16 PM Cleber Rosa <crosa@redhat.com> 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.utils" module to host the utility > > function and a test. > > > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > > Reviewed-by: Wainer dos Santos Moschetta <wainersm@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 | 21 ++++---------- > > tests/vm/basevm.py | 7 ++--- > > 5 files changed, 78 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 > > + """ > > + 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') > > This pattern is repeated every time a test needs to call > get_info_usernet_hostfwd_port. Do you see any downside on moving this > line inside the function and passing self.vm as an argument? This > would abstract the need to run info usernet before calling the > function. > My original plan was to follow this up with a utility in QEMUMachine itself (because that is the vm). It can still be done, but: * I don't expect *tests* to be calling this often, rather a base class for tests; * I'm avoiding changing QEMUMachine too much, given it's a more generic class and used by other tests (iotests, vm, etc). Hopefully when we have a better laid out structure for adding tests to QEMUMachine itself, it'll be confortable to change it more drastically. > > + 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 6dbd02d49d..052008f02d 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 archive > > from avocado.utils import ssh > > > > +from qemu.utils import get_info_usernet_hostfwd_port > > + > > > > class LinuxSSH(Test): > > > > @@ -70,18 +72,14 @@ def get_kernel_info(self, endianess, wordsize): > > 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") > > 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 ca64b76301..57a7047342 100644 > > --- a/tests/acceptance/virtiofs_submounts.py > > +++ b/tests/acceptance/virtiofs_submounts.py > > @@ -9,6 +9,8 @@ > > from avocado_qemu import wait_for_console_pattern > > from avocado.utils import ssh > > > > +from qemu.utils import get_info_usernet_hostfwd_port > > + > > > > def run_cmd(args): > > subp = subprocess.Popen(args, > > @@ -73,27 +75,14 @@ class VirtiofsSubmountsTest(LinuxTest): > > :avocado: tags=accel:kvm > > """ > > > > - 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 @@ > > 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 @@ def boot(self, img, extra_args=[]): > > 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) > > -- > > 2.25.4 > > > > Other than maybe a small adjustment to the > get_info_usernet_hostfwd_port function: > > Reviewed-by: Willian Rampazzo <willianr@redhat.com> > Thanks for the review! - Cleber.
On 3/23/21 6:15 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.utils" module to host the utility > function and a test. > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > Reviewed-by: Wainer dos Santos Moschetta <wainersm@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 | 21 ++++---------- > tests/vm/basevm.py | 7 ++--- > 5 files changed, 78 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 I think, unless you know something I don't, that I would prefer to keep type information in the "live" annotations where they can be checked against rot. > + """ > + 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 I wonder if more guest-specific code doesn't belong elsewhere, but I don't have a strong counter-suggestion, so I would probably ACK this for now. (Are you okay with the idea that we won't include the utils module in the PyPI upload? I think I would like to avoid shipping something like this outside of our castle walls, but agree that having it in the common code area somewhere for our own use is good.) > 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 6dbd02d49d..052008f02d 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 archive > from avocado.utils import ssh > > +from qemu.utils import get_info_usernet_hostfwd_port > + > > class LinuxSSH(Test): > > @@ -70,18 +72,14 @@ def get_kernel_info(self, endianess, wordsize): > 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") > 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 ca64b76301..57a7047342 100644 > --- a/tests/acceptance/virtiofs_submounts.py > +++ b/tests/acceptance/virtiofs_submounts.py > @@ -9,6 +9,8 @@ > from avocado_qemu import wait_for_console_pattern > from avocado.utils import ssh > > +from qemu.utils import get_info_usernet_hostfwd_port > + > > def run_cmd(args): > subp = subprocess.Popen(args, > @@ -73,27 +75,14 @@ class VirtiofsSubmountsTest(LinuxTest): > :avocado: tags=accel:kvm > """ > > - 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 @@ > 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 @@ def boot(self, img, extra_args=[]): > 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 Thu, Mar 25, 2021 at 02:10:19PM -0400, John Snow wrote: > On 3/23/21 6:15 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.utils" module to host the utility > > function and a test. > > > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > > Reviewed-by: Wainer dos Santos Moschetta <wainersm@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 | 21 ++++---------- > > tests/vm/basevm.py | 7 ++--- > > 5 files changed, 78 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 > > I think, unless you know something I don't, that I would prefer to keep type > information in the "live" annotations where they can be checked against rot. > No, that's a good point. No need to have type information defined twice. > > + """ > > + 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 > > I wonder if more guest-specific code doesn't belong elsewhere, but I don't > have a strong counter-suggestion, so I would probably ACK this for now. > There are multiple users of this pattern, and they go beyond the acceptance tests, so I think unifying them is a bit more important then having a better location. Also, like you, I can't think, of a better place at this time. > (Are you okay with the idea that we won't include the utils module in the > PyPI upload? I think I would like to avoid shipping something like this > outside of our castle walls, but agree that having it in the common code > area somewhere for our own use is good.) > At this time I don't have a need for it in the PyPI upload, but I wonder if this exception is justified. I mean, what would be gained, besides dealing with the exception itself, by not including it? Thanks for the feedback! - Cleber
On 4/11/21 10:09 PM, Cleber Rosa wrote: > At this time I don't have a need for it in the PyPI upload, but I > wonder if this exception is justified. I mean, what would be gained, > besides dealing with the exception itself, by not including it? > I just don't want to support or maintain little one-off misc utilities and things, but also don't wish to impose a larger design burden on you to integrate it in a more holistic and subjectively pleasant way. Having a misc/utils package where we just stick "kinda-sorta" stuff but never intend to ship or support is a useful middle ground. It's for your benefit so that we don't have to agonize about the interfaces, but still create common code that the rest of the QEMU tree can use. Even shipping 0.x stuff, releasing it onto PyPI imposes quite a design burden. At least within the QEMU tree I can see who is using which interfaces and how and avoid breakage. Once we pull that lever ... we won't have that benefit anymore. --js
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 6dbd02d49d..052008f02d 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 archive from avocado.utils import ssh +from qemu.utils import get_info_usernet_hostfwd_port + class LinuxSSH(Test): @@ -70,18 +72,14 @@ def get_kernel_info(self, endianess, wordsize): 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") 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 ca64b76301..57a7047342 100644 --- a/tests/acceptance/virtiofs_submounts.py +++ b/tests/acceptance/virtiofs_submounts.py @@ -9,6 +9,8 @@ from avocado_qemu import wait_for_console_pattern from avocado.utils import ssh +from qemu.utils import get_info_usernet_hostfwd_port + def run_cmd(args): subp = subprocess.Popen(args, @@ -73,27 +75,14 @@ class VirtiofsSubmountsTest(LinuxTest): :avocado: tags=accel:kvm """ - 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 @@ 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 @@ def boot(self, img, extra_args=[]): 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)