>From my reading of the current summary code it doesn't handle cases of malicious tests using shell expansion.
On the other hand I'm certain that my code does not do *any* expansion, so you would end up with folders named '/home/foo/tests/~shaders/$HOME/blah/blah/blah.html', since python doesn't expand *anything* implicitly, and I have not used os.path.expandvars() or os.path.expanduser() Otherwise I'll make the changes. On Sat, May 25, 2013 at 3:00 AM, Kenneth Graunke <[email protected]>wrote: > On 05/17/2013 09:31 AM, Dylan Baker wrote: > >> With 10,000+ tests all living in the same folder it can be hard to >> identify a single file, especially when trying to work with the HTML >> generation itself. This patch changes the behavior so that each test >> has a directory tree for the group results with tests under it (ie >> spec/GLSL 1.0/foo/bar/test.html) >> >> A side effect of this is that the group names of tests don't have to be >> altered to create a valid name (since the slashes in the group names >> just become the slashes in the folder structure) and this was the single >> most expensive operation of HTML generation (about 1/3 of the total >> time). This is a tidy little speedup. >> > > I'm nervous about skipping sanitizing of output filenames entirely. It > seems kind of dangerous. For example, I could (accidentally or > maliciously) create a file named tests/shaders/~.shader_test or > tests/shaders/$HOME.frag and it'll automatically create tests of similar > names. I suppose since '/' can't be part of a filename, it might work out > fine, but I'd rather not have to think about it... > > Regarding the speed, as I mentioned earlier, I think your implementation > of sanitizePath could be a lot more efficient. tr/translate would be much > faster, for example. > > You could probably also just sanitize it once (instead of twice) and pass > the path name around. Dunno. > > I'm not opposed to putting things in subdirectories. > > > Signed-off-by: Dylan Baker <[email protected]> >> --- >> framework/summary.py | 76 ++++++++++++++++--------------** >> ---------------- >> templates/test_result.mako | 6 ++-- >> 2 files changed, 30 insertions(+), 52 deletions(-) >> >> diff --git a/framework/summary.py b/framework/summary.py >> index 867dbad..9f2a924 100644 >> --- a/framework/summary.py >> +++ b/framework/summary.py >> @@ -472,8 +472,7 @@ class BuildHTMLIndex(list): >> """ >> self.append({'type': 'testResult', >> 'class': text, >> - 'href': path.join(self._sanitizePath(**group), >> - self._sanitizePath(href + >> ".html")), >> + 'href': path.join(group, href + ".html"), >> 'text': text}) >> >> def _subtestResult(self, group, text): >> @@ -489,29 +488,6 @@ class BuildHTMLIndex(list): >> 'class': text, >> 'text': text}) >> >> - # This will be removed from here in a later patch >> - def _sanitizePath(self, path): >> - """ >> - Private: convert the testname to only contain valid characters >> for >> - a path >> - >> - This attempts to faithfully reconstruct the output of >> - pigli-summary-html.**testPathToHtmlFilename, but without the >> use of >> - lambda. The advantage to not using lambda is readability, and the >> - desire to make future changes to the way that sub-tests are >> written >> - """ >> - valid_chars = frozenset("_-.%s%s" % (string.ascii_letters, >> - string.digits)) >> - out = [] >> - for c in path: >> - if c in valid_chars: >> - out.append(c) >> - elif c == "/": >> - out.append("__") >> - else: >> - out.append("_") >> - return ''.join(out) >> - >> >> class NewSummary: >> """ >> @@ -635,10 +611,17 @@ class NewSummary: >> output_encoding = "utf-8", >> module_directory = ".makotmp") >> >> + # Although these are only referenced once, that refernce is in a >> loop >> + # and they are less expensive to find once and store than find >> + # 10,000+ times >> > > This is kind of a "You don't say" moment - not doing things in a loop is > of course faster. I don't think you need to explain it. If you'd like to > keep the comment, there's a typo: "reference" > > > + resultCss = path.join(destination, "result.css") >> + index = path.join(destination, "index.html") >> + >> # Iterate across the tests creating the various test specific >> files >> for each in self.results: >> os.mkdir(path.join(**destination, each.name)) >> >> + # Create an index file for each test run >> file = open(path.join(destination, each.name, >> "index.html"), 'w') >> file.write(testindex.render(**name = each.name, >> time = each.time, >> @@ -649,15 +632,32 @@ class NewSummary: >> >> # Then build the individual test results >> for key, value in each.tests.iteritems(): >> + >> + # This is also improves performance, by cutting the >> number of >> + # times path.join and path.dirname is by 3, that's >> 20,000+ >> + # times! >> > > This English is very broken. I'm not sure it needs explaining either. > > > + tPath = path.join(destination, each.name, >> path.dirname(key)) >> + >> + # os.makedirs is very annoying, it throws an OSError if >> the >> + # path requested already exists, so do this check to >> esnure >> + # that it doesn't >> + if not path.exists(tPath): >> + os.makedirs(tPath) >> > > If you want, you could use checkDir. But this is also fine. > "ensure" (typo) > > > + >> file = open(path.join(destination, >> each.name, >> - self._sanitizePath(key + >> ".html")), 'w') >> + key + ".html"), >> + 'w') >> file.write(testfile.render(**testname = key, >> status = value['result'], >> returncode = >> value['returncode'], >> time = value['time'], >> info = value['info'], >> - command = >> value['command'])) >> + command = value['command'], >> + css = >> path.relpath(resultCss, >> + >> tPath), >> + index = index)) >> + >> file.close() >> > > I'd probably use "with open(...) as file:" syntax here, but that's just > me...it's your call. > > > >> # Finally build the root html files: index, regressions, etc >> @@ -811,25 +811,3 @@ class NewSummary: >> assert(len(currentStatus) == 1) >> >> return counts, status >> - >> - def _sanitizePath(self, path): >> - """ >> - Private: convert the testname to only contain valid characters >> for >> - a path >> - >> - This attempts to faithfully reconstruct the output of >> - pigli-summary-html.**testPathToHtmlFilename, but without the >> use of >> - lambda. The advantage to not using lambda is readability, and the >> - desire to make future changes to the way that sub-tests are >> written >> - """ >> - valid_chars = frozenset("_-.%s%s" % (string.ascii_letters, >> - string.digits)) >> - out = [] >> - for c in path: >> - if c in valid_chars: >> - out.append(c) >> - elif c == "/": >> - out.append("__") >> - else: >> - out.append("_") >> - return ''.join(out) >> diff --git a/templates/test_result.mako b/templates/test_result.mako >> index 1f944cb..7de0af7 100644 >> --- a/templates/test_result.mako >> +++ b/templates/test_result.mako >> @@ -5,7 +5,7 @@ >> <head> >> <meta http-equiv="Content-Type" content="text/html; >> charset=UTF-8" /> >> <title>${testname} - Details</title> >> - <link rel="stylesheet" href="../result.css" >> type="text/css" /> >> + <link rel="stylesheet" href="${css}" type="text/css" /> >> </head> >> <body> >> <h1>Results for ${testname}</h1> >> @@ -14,7 +14,7 @@ >> <p><b>Status:</b> ${status}</p> >> <p><b>Result:</b> ${status}</p> >> </div> >> - <p><a href="../index.html">Back to summary</a></p> >> + <p><a href="${index}">Back to summary</a></p> >> <h2>Details</h2> >> <table> >> <tr> >> @@ -42,6 +42,6 @@ >> </td> >> </tr> >> </table> >> - <p><a href="../index.html">Back to summary</a></p> >> + <p><a href="${index}">Back to summary</a></p> >> </body> >> </html> >> >> >
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
