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

From: Michael Goldish <mgoldish@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: autotest@test.kernel.org, lmr@redhat.com, kvm@vger.kernel.org
Subject: Re: [PATCH 5/9] KVM test: Log the content from guest serial console
Date: Wed, 28 Apr 2010 16:26:24 +0300	[thread overview]
Message-ID: <4BD83780.2040600@redhat.com> (raw)
In-Reply-To: <20100426100402.26268.22330.stgit@localhost.localdomain>

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.

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].

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

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