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 <jasow...@redhat.com>
>>> Signed-off-by: Amos Kong <ak...@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?
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):
>>> """
