Re: [Qemu-devel] [RFC PATCH 03/14] KVM Test: Add a common ping module for network related tests

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Tue, 2010-07-20 at 09:35 +0800, Amos Kong wrote:
> The kvm_net_utils.py is a just a place that wraps common network
> related commands which is used to do the network-related tests.
> Use -1 as the packet ratio for loss analysis.
> Use quiet mode when doing the flood ping.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  0 files changed, 0 insertions(+), 0 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_net_utils.py b/client/tests/kvm/kvm_net_utils.py
> index ede4965..8a71858 100644
> --- a/client/tests/kvm/kvm_net_utils.py
> +++ b/client/tests/kvm/kvm_net_utils.py
> @@ -1,4 +1,114 @@
> -import re
> +import logging, re, signal
> +from autotest_lib.client.common_lib import error
> +import kvm_subprocess, kvm_utils
> +
> +def get_loss_ratio(output):
> +    """
> +    Get the packet loss ratio from the output of ping
> +
> +    @param output
> +    """
> +    try:
> +        return int(re.findall('(\d+)% packet loss', output)[0])
> +    except IndexError:
> +        logging.debug(output)
> +        return -1

> +def raw_ping(command, timeout, session, output_func):
> +    """
> +    Low-level ping command execution.
> +
> +    @param command: ping command
> +    @param timeout: timeout of the ping command
> +    @param session: local executon hint or session to execute the ping command
> +    """
> +    if session == "localhost":
> +        process = kvm_subprocess.run_bg(command, output_func=output_func,
> +                                        timeout=timeout)

Do we really have to resort to kvm_subprocess here? We use subprocess on
special occasions, usually when the command in question is required to
live throughout the tests, which doesn't seem to be the case.
kvm_subprocess is a good library, but it is a more heavyweight
alternative (spawns its own shell process, for example). Maybe you are
using it because you can directly send input to the process at any time,
such as the ctrl+c later on this same code.

> +
> +        # Send SIGINT singal to notify the timeout of running ping process,

^ typo, signal

> +        # Because ping have the ability to catch the SIGINT signal so we can
> +        # always get the packet loss ratio even if timeout.
> +        if process.is_alive():
> +            kvm_utils.kill_process_tree(process.get_pid(), signal.SIGINT)
> +
> +        status = process.get_status()
> +        output = process.get_output()
> +
> +        process.close()
> +        return status, output
> +    else:
> +        session.sendline(command)
> +        status, output = session.read_up_to_prompt(timeout=timeout,
> +                                                   print_func=output_func)
> +        if status is False:

^ For None objects, it is better to explicitly test for is None. However
the above construct seems more natural if put as

if not status:

Any particular reason you tested explicitely for False?

> +            # Send ctrl+c (SIGINT) through ssh session
> +            session.sendline("\003")
> +            status, output2 = session.read_up_to_prompt(print_func=output_func)
> +            output += output2
> +            if status is False:
> +                # We also need to use this session to query the return value
> +                session.sendline("\003")
> +
> +        session.sendline(session.status_test_command)
> +        s2, o2 = session.read_up_to_prompt()
> +        if s2 is False:

^ See comment above

> +            status = -1
> +        else:
> +            try:
> +                status = int(re.findall("\d+", o2)[0])
> +            except:
> +                status = -1
> +
> +        return status, output
> +
> +def ping(dest = "localhost", count = None, interval = None, interface = None,
> +         packetsize = None, ttl = None, hint = None, adaptive = False,
> +         broadcast = False, flood = False, timeout = 0,
> +         output_func = logging.debug, session = "localhost"):

^ On function headers, we pretty much follow PEP 8 and don't use spacing
between argument keys, the equal sign and the default value, so this
header should be rewritten

def ping(dest="localhost",...)

> +    """
> +    Wrapper of ping.
> +
> +    @param dest: destination address
> +    @param count: count of icmp packet
> +    @param interval: interval of two icmp echo request
> +    @param interface: specified interface of the source address
> +    @param packetsize: packet size of icmp
> +    @param ttl: ip time to live
> +    @param hint: path mtu discovery hint
> +    @param adaptive: adaptive ping flag
> +    @param broadcast: broadcast ping flag
> +    @param flood: flood ping flag
> +    @param timeout: timeout for the ping command
> +    @param output_func: function used to log the result of ping
> +    @param session: local executon hint or session to execute the ping command
> +    """
> +
> +    command = "ping %s " % dest
> +
> +    if count is not None:
> +        command += " -c %s" % count
> +    if interval is not None:
> +        command += " -i %s" % interval
> +    if interface is not None:
> +        command += " -I %s" % interface
> +    if packetsize is not None:
> +        command += " -s %s" % packetsize
> +    if ttl is not None:
> +        command += " -t %s" % ttl
> +    if hint is not None:
> +        command += " -M %s" % hint
> +    if adaptive is True:
> +        command += " -A"
> +    if broadcast is True:
> +        command += " -b"
> +    if flood is True:
> +        # temporary workaround as the kvm_subprocess may not properly handle
> +        # the timeout for the output of flood ping
> +        command += " -f -q"
> +        output_func = None

^ the last 3 if clauses could be simpler if put as:

if adaptive:

if broadcast:

if flood:

> +    return raw_ping(command, timeout, session, output_func)
>  
>  def get_linux_ifname(session, mac_address):
>      """
> 
>