piglit never clears the ringbuffer, all of the concern is that we handle the ringbuffer being empty at startup or emptied durring the run by a user correctly.
On Tuesday, February 04, 2014 12:40:26 PM Ken Phillis Jr wrote: > I believe that this should be tested for permissions checks. I > honestly do not believe that it would be a good idea to always clear > the ringbuffer seeing as how on some systems this command requires > root level permissions. > > On Tue, Feb 4, 2014 at 12:28 PM, Ilia Mirkin <[email protected]> wrote: > > On Tue, Feb 4, 2014 at 1:12 PM, Dylan Baker <[email protected]> wrote: > >> On Tuesday, February 04, 2014 12:43:42 PM Ilia Mirkin wrote: > >>> On Tue, Feb 4, 2014 at 12:35 PM, Dylan Baker <[email protected]> > >> > >> 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 > >>> > separate 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. > >>> > > >>> > v2: - Remove unnecessary assert from PosixDmesg.update_result() > >>> > > >>> > - simplify replace helper in PoixDmesg.update_result() > >>> > - replace try/except with if check > >>> > - Change PosixDmesg.update_dmesg() to work even if dmesg 'wraps' > >>> > > >>> > v3: - Try/Except assignment of PosixDmesg._last_message in > >>> > update_dmesg() > >>> > > >>> > - Set the dmesg levels the same as the previous implementation > >>> > - Change PosixDmesg into LinuxDmesg and enforce that dmesg has > >>> > > >>> > timestamps > >>> > > >>> > v4: - Add check in LinuxDmesg.__init__ to ensure there is something in > >>> > > >>> > dmesg before doing the timestamp check. This should make > >>> > problems > >>> > clearer > >>> > > >>> > - Refactor some code in LinuxDmesg.update_result() > >>> > > >>> > v5: - Replace exception with warning. > >>> > > >>> > - add break to loop in dmesg.update_dmesg > >>> > > >>> > Signed-off-by: Dylan Baker <[email protected]> > >>> > --- > >>> > > >>> > framework/dmesg.py | 175 > >>> > +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, > >>> > 175 insertions(+) > >>> > create mode 100644 framework/dmesg.py > >>> > > >>> > diff --git a/framework/dmesg.py b/framework/dmesg.py > >>> > new file mode 100644 > >>> > index 0000000..feb05ad > >>> > --- /dev/null > >>> > +++ b/framework/dmesg.py > >>> > @@ -0,0 +1,175 @@ > >>> > +# 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 > >>> > + > >>> > +Currently this module only has the default DummyDmesg, and a > >>> > LinuxDmesg. > >>> > +The method used on Linux requires that timetamps are enabled, and no > >>> > other > >>> > +posix system has timestamps. > >>> > + > >>> > +On OSX and *BSD one would likely want to implement a system that > >>> > reads > >>> > the > >>> > +sysloger, since timestamps can be added by the sysloger, and are not > >>> > inserted +by dmesg. > >>> > + > >>> > +""" > >>> > + > >>> > +import re > >>> > +import sys > >>> > +import subprocess > >>> > +import warnings > >>> > + > >>> > +__all__ = ['LinuxDmesg', 'DummyDmesg', 'get_dmesg'] > >>> > + > >>> > + > >>> > +class LinuxDmesg(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. It requires timestamps to be enabled. > >>> > + > >>> > + """ > >>> > + DMESG_COMMAND = ['dmesg', '--level', > >>> > 'emerg,alert,crit,err,warn,notice'] + > >>> > + def __init__(self): > >>> > + """ Create a dmesg instance """ > >>> > + self._last_message = None > >>> > + self._new_messages = [] > >>> > + > >>> > + # Populate self.dmesg initially, otherwise the first test > >>> > will > >>> > always + # be full of dmesg crud. > >>> > + self.update_dmesg() > >>> > + > >>> > + if not self._last_message: > >>> > + # We need to ensure that there is something in dmesg to > >>> > check > >>> > + # timestamps. > >>> > + warnings.warn("Cannot check dmesg for timestamp support. > >>> > If > >>> > you " + "do not have timestamps enabled in > >>> > your > >>> > kernel you " + "get incomplete dmesg > >>> > captures", > >>> > RuntimeWarning) + elif not re.match(r'\[\s*\d+\.\d+\]', > >>> > self._last_message): + # Do an initial check to ensure that > >>> > dmesg has timestamps, we need + # timestamps to work > >>> > correctly. A proper linux dmesg timestamp + # looks like > >>> > this: > >>> > [ 0.00000] > >>> > + raise DmesgError( > >>> > + "Your kernel does not seem to support timestamps, > >>> > which " > >>> > + "are required by the --dmesg option") > >>> > >>> Consider also making this a warning. > >>> > >>> Either way, Reviewed-by: Ilia Mirkin <[email protected]> > >>> > >>> > + > >>> > + 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 > >>> > + > >>> > + """ > >>> > + > >>> > + def replace(res): > >>> > + """ helper to replace statuses with the new dmesg status > >>> > + > >>> > + Replaces pass with dmesg-warn, and warn and fail with > >>> > dmesg-fail, + otherwise returns the input > >>> > + > >>> > + """ > >>> > + return {"pass": "dmesg-warn", > >>> > + "warn": "dmesg-fail", > >>> > + "fail": "dmesg-fail"}.get(res, res) > >>> > + > >>> > + # 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 > >>> > + if 'subtest' in result: > >>> > + for key, value in result['subtest'].iteritems(): > >>> > + result['subtest'][key] = replace(value) > >>> > + > >>> > + # 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).strip().splitlines() + > >>> > + # Find all new entries, do this by slicing the list of dmesg > >>> > to > >>> > only + # returns elements after the last element stored. If > >>> > there > >>> > are not + # matches a value error is raised, that means all of > >>> > dmesg is new + l = 0 > >>> > + for index, item in enumerate(reversed(dmesg)): > >>> > + if item == self._last_message: > >>> > + l = len(dmesg) - index # don't include the matched > >>> > element + break > >>> > + self._new_messages = dmesg[l:] > >>> > + > >>> > + # Attempt to store the last element of dmesg, unless there > >>> > was no > >>> > dmesg + self._last_message = dmesg[-1] if dmesg else None > >>> > + > >>> > + > >>> > +class DummyDmesg(object): > >>> > + """ An dummy class for dmesg on non unix-like systems > >>> > + > >>> > + This implements a dummy version of the LinuxDmesg, 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 > >>> > + > >>> > + > >>> > +class DmesgError(EnvironmentError): > >>> > + pass > >>> > + > >>> > + > >>> > +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 sys.platform.startswith('linux') and not_dummy: > >>> > + return LinuxDmesg() > >>> > + return DummyDmesg() > >>> > -- > >>> > 1.8.5.3 > >>> > > >>> > _______________________________________________ > >>> > Piglit mailing list > >>> > [email protected] > >>> > http://lists.freedesktop.org/mailman/listinfo/piglit > >> > >> Is that review for the series? > > > > It's a review for this patch. I believe I also reviewed 3/4 and 4/4 > > earlier on (and those reviews should still stand). 2/4 uses some fancy > > testing framework I'm not familiar with, so I haven't looked at it. > > But the impact is also low, so probably fine to commit unreviewed? > > Dunno what the policy is. > > _______________________________________________ > > Piglit mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/piglit
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
