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

[Michael Goldish]

On 07/28/2010 02:50 PM, Michael Goldish wrote:
> 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 <address@hidden>
>>> Signed-off-by: Amos Kong <address@hidden>
>>> ---
>>>  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?

Please disregard this comment.  After looking at some of the other
patches in the set I realized it wasn't relevant.

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