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@xxxxxxxxxx>
>> Signed-off-by: Amos Kong <akong@xxxxxxxxxx>
>> ---
>> 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):
>> """
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
