Re: [PATCH v2 10/10] KVM test: Add a helper to search the panic in the log

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Mon, 2010-05-17 at 15:28 +0800, Jason Wang wrote:
> Michael Goldish wrote:
> > On 05/11/2010 12:04 PM, Jason Wang wrote:
> >   
> >> This checker serves as the post_command to find the panic information
> >> in the file which contains the content of guest serial console.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>  client/tests/kvm/scripts/check_serial.py |   41 ++++++++++++++++++++++++++++++
> >>  client/tests/kvm/tests_base.cfg.sample   |    7 ++++-
> >>  2 files changed, 46 insertions(+), 2 deletions(-)
> >>  create mode 100644 client/tests/kvm/scripts/check_serial.py
> >>
> >> diff --git a/client/tests/kvm/scripts/check_serial.py b/client/tests/kvm/scripts/check_serial.py
> >> new file mode 100644
> >> index 0000000..969bbe3
> >> --- /dev/null
> >> +++ b/client/tests/kvm/scripts/check_serial.py
> >> @@ -0,0 +1,41 @@
> >> +import os, sys, glob, re
> >> +
> >> +
> >> +class SerialCheckerError(Exception):
> >> +    """
> >> +    Simple wrapper for the builtin Exception class.
> >> +    """
> >> +    pass
> >> +
> >> +
> >> +class SerialChecker(object):
> >> +    """
> >> +    Serach the panic or other keywords in the guest serial.

^ Typo here, search.

> >> +    """
> >> +    def __init__(self):
> >> +        """
> >> +        Gets params from environment variables and sets class attributes.
> >> +        """
> >> +        client_dir =  os.environ['AUTODIR']
> >> +        self.pattern = os.environ['KVM_TEST_search_pattern']
> >> +        self.shortname = os.environ['KVM_TEST_shortname']
> >> +        self.debugdir = os.path.join(client_dir, "results/default/kvm.%s/debug" \
> >>     
> >
> > I think the final backslash is unnecessary.
> >
> >   
> >> +                                     % self.shortname)
> >> +        self.serial_files = glob.glob(os.path.join(self.debugdir, 'serial*'))
> >> +
> >> +
> >> +    def check(self):
> >> +        """
> >> +        Check whether the pattern were found in the serial files
> >> +        """
> >> +        fail = [ f for f in self.serial_files if
> >> +                 re.findall(self.pattern, file(f).read(), re.I) ]
> >> +        if fail:
> >> +            print "%s is found in %s" % (self.pattern, fail)
> >> +            raise SerialCheckerError("Error found during the check, Please" \
> >>     
> >
> > Same here.
> >
> >   
> >> +                                     " check the log")
> >> +
> >> +
> >> +if __name__ == "__main__":
> >> +    checker = SerialChecker()
> >> +    checker.check()
> >>     
> >
> > I wonder why we need a class.  Why not just put all the code here?
> >
> >   
> Just follow the style of other pre_command, maybe Lucas like it.

When I made the first pre command script, I opted to use a class to
represent the unattended install setup, and other pre commands followed
this style. Since the code is small, I see no problem in putting it
under __main__ altogether. So Jason, you can remove the code from the
class and put it under __main__, please.

> >> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> >> index 3c0933e..3ac8f0d 100644
> >> --- a/client/tests/kvm/tests_base.cfg.sample
> >> +++ b/client/tests/kvm/tests_base.cfg.sample
> >> @@ -52,6 +52,10 @@ address_index = 0
> >>  # Misc
> >>  profilers = kvm_stat
> >>  
> >> +# pattern serach in guest serial console
> >> +serach_pattern = panic

^ typo, should be search_pattern

> >> +post_command = "python scripts/check_serial.py"
> >> +post_command_noncritical = no
> >>  
> >>  # Tests
> >>  variants:
> >> @@ -1314,10 +1318,9 @@ virtio|virtio_blk|e1000|balloon_check:
> >>  variants:
> >>      - @qcow2:
> >>          image_format = qcow2
> >> -        post_command = " python scripts/check_image.py;"
> >> +        post_command = " python scripts/check_image.py; python scripts/check_serial.py"
> >>     
> >                         ^
> > This should be +=, because post_command may have been previously
> > assigned some value.
> >
> >   
> Would change it, and do you have any other comments about this patchset?

Other than the issues pointed out, looks good.