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

[Cleber Rosa]

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

signature.asc
Description: PGP signature