-
Notifications
You must be signed in to change notification settings - Fork 248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow handling of compressed perfparser files #663
base: master
Are you sure you want to change the base?
Conversation
all of the code was in place but checking for perfparser format used the original filename instead the temporary (unpacked) one
"Linux Perf Files (perf*.data perf.data.*);;" | ||
"Perfparser Files (*.perfparser);;" | ||
"Perfparser Files (*.perfparser *.perfparser.*);;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it an explicit list of supported extensions, and only when karchive was found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my thought, too; but where can I find the list of supported extensions (and is it always an "all extensions or KFArchive not set")?
@@ -1614,7 +1614,7 @@ void PerfParser::startParseFile(const QString& path) | |||
|
|||
// note: file is always readable and in supported format here, | |||
// already validated in initParserArgs() | |||
QFile file(path); | |||
QFile file(m_parserArgs[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not thread safe. and why is this not using path anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path
is unchanged (we'd need an additional object or return a QString from initParserArgs()
, the changed version is only placed into the m_parserArgs[]
.
Please drop a note how you would like this to be tackled and I look into doing that.
But in any case I'm interested why this isn't trhead safe - the code that adjusts m_parserArgs
is always executed up-front, no?
@@ -234,6 +234,9 @@ private slots: | |||
|
|||
const auto perfData = QFINDTESTDATA("file_content/true.perfparser"); | |||
QTest::addRow("pre-exported perfparser") << perfData << QString(); | |||
#if KFArchive_FOUND | |||
QTest::addRow("perfparser, xzipped") << QFINDTESTDATA("file_content/true.perfparser.xz") << QString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zstd, gz, ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do you want to test the other formats = testing KFArchive here as well?
all of the code was in place but checking for perfparser format used the original filename instead the temporary (unpacked) one
I guess that xz format is always available when KFArchive_FOUND is set - but if this isn't the case I'll change the test from xz to gz.
note: I'm open to suggestions for accessing the filename different than via
m_parserArgs[1]
...