Re: [PATCH 5/9] KVM test: Log the content from guest serial console

[Jason Wang <jasowang@redhat.com>]

Michael Goldish wrote:
> On 04/26/2010 01:04 PM, Jason Wang wrote:
>   
>> This patch tries to get the content of guest serial and log it into
>> the debug directoy of the testcase through a dedicated thread which is
>> created in the preprocessing and ended in the postprocessing. The
>> params of serial_mode must be set to "dump" in order to make use of
>> this feature.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  client/tests/kvm/kvm_preprocessing.py  |   59 +++++++++++++++++++++++++++++++-
>>  client/tests/kvm/tests_base.cfg.sample |    1 +
>>  2 files changed, 59 insertions(+), 1 deletions(-)
>>
>> diff --git a/client/tests/kvm/kvm_preprocessing.py b/client/tests/kvm/kvm_preprocessing.py
>> index 4b9290c..50d0e35 100644
>> --- a/client/tests/kvm/kvm_preprocessing.py
>> +++ b/client/tests/kvm/kvm_preprocessing.py
>> @@ -1,4 +1,5 @@
>>  import sys, os, time, commands, re, logging, signal, glob, threading, shutil
>> +import socket, select
>>  from autotest_lib.client.bin import test, utils
>>  from autotest_lib.client.common_lib import error
>>  import kvm_vm, kvm_utils, kvm_subprocess, ppm_utils
>> @@ -13,7 +14,8 @@ except ImportError:
>>  
>>  _screendump_thread = None
>>  _screendump_thread_termination_event = None
>> -
>> +_serialdump_thread = None
>> +_serialdump_thread_termination_event = None
>>  
>>  def preprocess_image(test, params):
>>      """
>> @@ -267,6 +269,16 @@ def preprocess(test, params, env):
>>                                                args=(test, params, env))
>>          _screendump_thread.start()
>>  
>> +    # Start the serial dump thread
>> +    if params.get("serial_mode") == "dump":
>> +        logging.debug("Starting serialdump thread")
>> +        global _serialdump_thread, _serialdump_thread_termination_event
>> +        _serialdump_thread_termination_event = threading.Event()
>> +        _serialdump_thread = threading.Thread(target=_dump_serial_console,
>> +                                              args=(test, params, env))
>> +        _serialdump_thread.start()
>> +
>> +
>>  
>>  def postprocess(test, params, env):
>>      """
>> @@ -286,6 +298,13 @@ def postprocess(test, params, env):
>>          _screendump_thread_termination_event.set()
>>          _screendump_thread.join(10)
>>  
>> +    # Terminate the serialdump thread
>> +    global _serialdump_thread, _serialdump_thread_termination_event
>> +    if _serialdump_thread:
>> +        logging.debug("Terminating serialdump thread...")
>> +        _serialdump_thread_termination_event.set()
>> +        _serialdump_thread.join(10)
>> +
>>      # Warn about corrupt PPM files
>>      for f in glob.glob(os.path.join(test.debugdir, "*.ppm")):
>>          if not ppm_utils.image_verify_ppm_file(f):
>> @@ -450,3 +469,41 @@ def _take_screendumps(test, params, env):
>>          if _screendump_thread_termination_event.isSet():
>>              break
>>          _screendump_thread_termination_event.wait(delay)
>> +
>> +def _dump_serial_console(test, params, env):
>> +    global _serialdump_thread_termination_event
>> +    rs = []
>> +    files = {}
>> +
>> +    while True:
>> +        for vm in kvm_utils.env_get_all_vms(env):
>> +            if not files.has_key(vm):
>>     
>
> You should probably add "and not vm.is_dead()" to this condition.
> Otherwise we'll get lots of "could not connect to serial socket"
> messages for dead VMs.
>   
Nice catch, thanks.
> Style note: AFAIK in the Autotest project the form "if not vm in files"
> is preferred.
>
>   
>> +                try:
>> +                    serial_socket = socket.socket(socket.AF_UNIX,
>> +                                                  socket.SOCK_STREAM)
>> +                    serial_socket.setblocking(False)
>> +                    serial_socket.connect(vm.serial_file_name)
>> +                except:
>> +                    logging.debug("Could not connect to serial socket for %s" %
>> +                                  vm.name)
>> +                    continue
>> +                rs.append(serial_socket)
>> +                serial_dump_filename = os.path.join(test.debugdir,
>> +                                                    "serial-%s" % vm.name)
>> +                files[vm] = [serial_socket, file(serial_dump_filename, "a+")]
>> +
>> +        r, w, x = select.select(rs, [], [], 0.5)
>> +        for vm in files.keys():
>> +            [s ,d] = files[vm]
>>     
>
> For consistency, please consider changing this list to a tuple, i.e.
> s, d = files[vm] or (s, d) = files[vm].
>
>   
Yes, tuple is better.
>> +            if s in r:
>> +                data = s.recv(16384)
>> +                if len(data) == 0:
>>     
>
> Style note: AFAIK the preferred form is "if not data".
>
> Sorry for the petty comments.  Overall the patch looks good.
>   
Thanks for the comments and please do not hesitate to point the defects.
>   
>> +                    rs.remove(s)
>> +                    files.pop(vm)
>> +                else:
>> +                    d.write(data)
>> +                
>> +        if _serialdump_thread_termination_event.isSet():
>> +            break
>> +
>> +            
>> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
>> index 9f82ffb..169a69e 100644
>> --- a/client/tests/kvm/tests_base.cfg.sample
>> +++ b/client/tests/kvm/tests_base.cfg.sample
>> @@ -13,6 +13,7 @@ start_vm = yes
>>  kill_vm = no
>>  kill_vm_gracefully = yes
>>  kill_unresponsive_vms = yes
>> +serial_mode = dump
>>  
>>  # Screendump specific stuff
>>  convert_ppm_files_to_png_on_error = yes
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>     
>
>