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

[Michael Goldish <mgoldish@redhat.com>]

On 07/27/2010 04:01 PM, Lucas Meneghel Rodrigues wrote:
> 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":

I have no problem with this, but wouldn't it be nicer to use None to
indicate that the session should be local?

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

read_up_to_prompt() returns True/False as the first member of the tuple.

>> +            # Send ctrl+c (SIGINT) through ssh session
>> +            session.sendline("\003")

I think you can use send() here.  sendline() calls send() with a newline
added to the string.

>> +            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")

Same here.

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

Don't we need this for Windows as well?  If we do, wouldn't it be easier
to use a parameter (e.g. 'ping_options = -c 3') to directly control the
ping options from the config file, instead of using an OS-specific ping
wrapper?

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

What do you mean by this?  If there's a problem with kvm_subprocess it
should be fixed.  Have you tried passing internal_timeout=0 to the
kvm_subprocess function you're using?

>> +        command += " -f -q"

If a lot of output is generated, it may not be a bad idea to use -q here
regardless of kvm_subprocess limitations.

>> +        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):
>>      """