[Piglit] [PATCH 1/4] framework/dmesg: Adds a new module for dmesg
Dylan Baker
baker.dylan.c at gmail.com
Fri Jan 31 23:11:51 PST 2014
On Friday, January 31, 2014 08:39:31 PM Ilia Mirkin wrote:
> On Fri, Jan 31, 2014 at 8:02 PM, Dylan Baker <baker.dylan.c at gmail.com>
wrote:
> > This modules implements two classes and a helper function. The two
> > classes are both dmesg wrapper classes, one, PosixDmesg is used on posix
> > systems when requested to actually do checks on dmesg. The second class,
> > DummyDmesg, is used to reduce code complexity by providing dummy
> > versions of the methods in PosixDmesg. This means that we don't need
> > seperate code paths for Posix systems wanting to check dmesg, Posix
> > systems not wanting to check dmesg, and non-posix systems which lack
> > dmesg.
> >
> > Beyond reducing complexity this module also gives better isolation,
> > dmesg is only used in Test.execute(), no where else. Additional classes
> > don't need to worry about dmesg that way, it's just available. The third
> > advantage is performance, this module doesn't search for timestamps and
> > compare from there, instead it creates snapshots of dmesg as lists, and
> > does a linear comparison between them.
> >
> > Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> > ---
> >
> > framework/dmesg.py | 157
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > 157 insertions(+)
> > create mode 100644 framework/dmesg.py
> >
> > diff --git a/framework/dmesg.py b/framework/dmesg.py
> > new file mode 100644
> > index 0000000..81410d6
> > --- /dev/null
> > +++ b/framework/dmesg.py
> > @@ -0,0 +1,157 @@
> > +# Copyright (c) 2013-2014 Intel Corporation
> > +#
> > +# Permission is hereby granted, free of charge, to any person obtaining a
> > +# copy of this software and associated documentation files (the
> > "Software"), +# to deal in the Software without restriction, including
> > without limitation +# the rights to use, copy, modify, merge, publish,
> > distribute, sublicense, +# and/or sell copies of the Software, and to
> > permit persons to whom the +# Software is furnished to do so, subject to
> > the following conditions: +#
> > +# The above copyright notice and this permission notice (including the
> > next +# paragraph) shall be included in all copies or substantial
> > portions of the +# Software.
> > +#
> > +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> > IN NO EVENT SHALL +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> > TORT OR OTHERWISE, ARISING +# FROM, OUT OF OR IN CONNECTION WITH THE
> > SOFTWARE OR THE USE OR OTHER DEALINGS +# IN THE SOFTWARE.
> > +
> > +""" Module implementing classes for reading posix dmesg """
> > +
> > +import os
> > +import subprocess
> > +import itertools
> > +
> > +__all__ = ['get_dmesg']
> > +
> > +
> > +class PosixDmesg(object):
> > + """ Read dmesg on posix systems
> > +
> > + This reads the dmesg ring buffer, stores the contents of the buffer,
> > and + then compares it whenever called, returning the difference
> > between the + calls.
> > +
> > + This class is not thread-safe, but with the caveat that the test
> > reporting + dmesg problems might not be the test that generated them.
> > dmesg reporting + can only be considered authoritative when running
> > non-concurrently. +
> > + """
> > + DMESG_COMMAND = ['dmesg']
> > +
> > + def __init__(self):
> > + """ Create a dmesg instance """
> > + self._dmesg = []
> > + self._last_message = ""
> > + self._new_messages = []
>
> I suppose a reason for not double-underscoring these will become apparent
> later?
There are two reasons I used the single underscore:
1) the double underscore is really about protecting members from children, not
privacy, and I don't really see any reason to subclass the PosixDmesg class.
2) It makes testing a pain, since I would need to ask for memebers as
_PosixDmesg__last_message, and that's annoying.
> > +
> > + # Populate self.dmesg initially, otherwise the first test will
> > always + # be full of dmesg crud.
> > + self.update_dmesg()
> > +
> > + def update_result(self, result):
> > + """ Takes a TestResult object and updates it with dmesg statuses
> > +
> > + If dmesg is enabled, and if dmesg has been updated, then replace
> > pass + with dmesg-warn and warn and fail with dmesg-fail. If dmesg
> > has not + been updated, it will return the original result passed
> > in. +
> > + Arguments:
> > + result -- A TestResult instance
> > +
> > + """
> > + assert(isinstance(result, dict))
>
> Doesn't _really_ have to be a dict, does it? And any plain ol' dict
> won't work either since you expect certain keys to be there. I'd drop
> this.
>
fair enough. dropped.
> > +
> > + def replace(res):
> > + """ helper to replace statuses with the new dmesg status
> > +
> > + Replaces pass with dmesg-warn, and warn and fail with
> > dmesg-fail. +
> > + """
> > + if res == "pass":
> > + return 'dmesg-warn'
> > + elif res in ['warn', 'fail']:
> > + return 'dmesg-fail'
> > + return res
>
> A slightly faster way to do this might be:
>
> return {
> "pass": "dmesg-warn",
> "warn": "dmesg-fail",
> "fail": "dmesg-fail",
> }.get(res, res)
>
> Although it's also a bit fancier, so perhaps not worth it.
That's a pattern I haven't seen before, but I like it.
>
> > +
> > + # Get a new snapshot of dmesg
> > + self.update_dmesg()
> > +
> > + # if update_dmesg() found new entries replace the results of the
> > test + # and subtests
> > + if self._new_messages:
> > + result['result'] = replace(result['result'])
> > +
> > + # Replace any subtests
> > + try:
> > + for key, value in result['subtest'].iteritems():
> > + result['subtest'][key] = replace(value)
> > + except KeyError:
> > + pass
>
> Is this really cleaner than
>
> for key, value in result.get('subtest', {}).iteritems()
>
> or
>
> if 'subtest' in result:
> for ...
>
> Or is the try/catch protecting against something subtle I'm not thinking of?
No, it's an artifact from having a custom exception at one point. I'll use if
'subtest':
> > +
> > + # Add the dmesg values to the result
> > + result['dmesg'] = self._new_messages
> > +
> > + return result
> > +
> > + def update_dmesg(self):
> > + """ Call dmesg using subprocess.check_output
> > +
> > + Get the contents of dmesg, then calculate new messages, finally
> > set + self.dmesg equal to the just read contents of dmesg.
> > +
> > + """
> > + dmesg = subprocess.check_output(self.DMESG_COMMAND).splitlines()
> > +
> > + # Calculate new entires in dmesg by returning any strings that
> > are in + # the new dmesg list but not in the old one. This
> > circumvents the + # ringbuffer issue, since if there are not
> > entries in common all of the + # new messages will be returned.
>
> Can you explain how this works? Let's say that dmesg can store 3 lines
> (yeah, i know it's bytes, but wtvr), self._dmesg would be a, b, c.
> Then a new line is emitted, and it becomes b, c, d. itertools.izip
> will have new/old pairs of (a, b), (b, c), (c, d) -- it'll all appear
> to be new. Am I misreading the code/misunderstanding how izip works?
> Basically I don't see a "try to line things up" step. Also note that
> because it actually _is_ bytes and not lines, the very first line
> might be truncated (its first characters, not its last ones).
>
You're complete right, if the ringbuffer wraps this will break. What I was
originally doing is this:
self._new_messages = [x for x in dmesg if x not in self._dmesg]
But Ken Graunke pointed out that this is an exponentially expensive operation.
And So, you're right I need a step to line them up. I'll work on that and send
out a v2.
> > + self._new_messages = []
> > + for new, old in itertools.izip_longest(dmesg, self._dmesg,
> > fillvalue=None): + if new == old:
> > + continue
> > + self._new_messages.append(new)
> > +
> > +
> > + # Now that we have checked the entries of self._dmesg, we can
> > update + # that member with the contents of the latest read of
> > dmesg + self._dmesg = dmesg
> > +
> > +
> > +class DummyDmesg(object):
> > + """ An dummy class for dmesg on non unix-like systems
> > +
> > + This implements a dummy version of the PosixDmesg, and can be used
> > anytime + it is not desirable to actually read dmesg, such as
> > non-posix systems, or + when the contents of dmesg don't matter.
> > +
> > + """
> > + DMESG_COMMAND = []
> > +
> > + def __init__(self):
> > + pass
> > +
> > + def update_dmesg(self):
> > + """ Dummy version of update_dmesg """
> > + pass
> > +
> > + def update_result(self, result):
> > + """ Dummy version of update_result """
> > + return result
> > +
> > +
> > +def get_dmesg(not_dummy=True):
> > + """ Return a Dmesg type instance
> > +
> > + Normally this does a system check, and returns the type that proper
> > for + your system. However, if Dummy is True then it will always
> > return a + DummyDmesg instance.
> > +
> > + """
> > + if os.name == "posix" and not_dummy:
> > + return PosixDmesg()
> > + return DummyDmesg()
> > --
> > 1.8.5.3
> >
> > _______________________________________________
> > Piglit mailing list
> > Piglit at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/piglit
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140131/f19e8c43/attachment-0001.pgp>
More information about the Piglit
mailing list