Adderssing the issues reported by "howaboutsynergy" here: Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204277 Yordan Karadzhov (VMware) (3): kernel-shark: Make KsEventsCheckBoxWidget::removeSystem more robust kernel-shark: Better error message for the constructor of KsCaptureControl kernel-shark: Disable Capture if trace-cmd can't function kernel-shark/src/KsCaptureDialog.cpp | 25 ++++++++++++++++++++++--- kernel-shark/src/KsWidgetsLib.cpp | 9 ++++++--- 2 files changed, 28 insertions(+), 6 deletions(-) -- 2.20.1
The function has to be able to handle safely the case when the Checkbox tree widget is empty or it does not contain the item to be removed. Reported-by: howaboutsynergy <howaboutsynergy@pm.me> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204277 Fixes: 4a02481fff (Remove all system=ftrace events from Record dialog) Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- kernel-shark/src/KsWidgetsLib.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/kernel-shark/src/KsWidgetsLib.cpp b/kernel-shark/src/KsWidgetsLib.cpp index 84afec9..330230e 100644 --- a/kernel-shark/src/KsWidgetsLib.cpp +++ b/kernel-shark/src/KsWidgetsLib.cpp @@ -749,10 +749,13 @@ QStringList KsEventsCheckBoxWidget::getCheckedEvents(bool option) /** Remove a System from the Checkbox tree. */ void KsEventsCheckBoxWidget::removeSystem(QString name) { - QTreeWidgetItem *item = - _tree.findItems(name, Qt::MatchFixedString, 0)[0]; + auto itemList = _tree.findItems(name, Qt::MatchFixedString, 0); + int index; - int index = _tree.indexOfTopLevelItem(item); + if (itemList.isEmpty()) + return; + + index = _tree.indexOfTopLevelItem(itemList[0]); if (index >= 0) _tree.takeTopLevelItem(index); } -- 2.20.1
The error message includes the case when the tracing directory cannot be found or mounted. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- kernel-shark/src/KsCaptureDialog.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp index 0a29518..2e6e8f9 100644 --- a/kernel-shark/src/KsCaptureDialog.cpp +++ b/kernel-shark/src/KsCaptureDialog.cpp @@ -55,13 +55,19 @@ KsCaptureControl::KsCaptureControl(QWidget *parent) _topLayout.addWidget(line); }; - if (pluginList.count() == 0) { + if (pluginList.count() == 0 || !_localTEP) { /* - * No plugins have been found. Most likely this is because - * the process has no Root privileges. + * No plugins or events have been found. Most likely this is + * because the process has no Root privileges or because + * tracefs cannot be mounted. */ QString message("Error: No events or plugins found.\n"); - message += "Root privileges are required."; + + if (!_localTEP) + message += "Cannot find or mount tracing directory.\n"; + if (!pluginList.count()) + message += "Root privileges are required.\n"; + QLabel *errorLabel = new QLabel(message); errorLabel->setStyleSheet("QLabel {color : red;}"); -- 2.20.1
In the case of an error all key buttons of the Record dialog are disabled. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- kernel-shark/src/KsCaptureDialog.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp index 2e6e8f9..2962917 100644 --- a/kernel-shark/src/KsCaptureDialog.cpp +++ b/kernel-shark/src/KsCaptureDialog.cpp @@ -74,6 +74,19 @@ KsCaptureControl::KsCaptureControl(QWidget *parent) _topLayout.addWidget(errorLabel); lamAddLine(); + + /* Disable all key buttons. */ + QVector<QWidget *> widgets = + {&_importSettingsButton, + &_exportSettingsButton, + &_outputBrowseButton, + &_pluginsComboBox, + &_commandCheckBox, + &_applyButton, + &_captureButton}; + + for (auto &b: widgets) + b->setDisabled(true); } pluginList.prepend("nop"); -- 2.20.1
On Tue, 23 Jul 2019 15:52:04 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > In the case of an error all key buttons of the Record dialog are > disabled. I applied and pushed out your other two patches, but didn't apply this one. As we discussed at our 1:1, I think it's better to at a minimum, just disable the "Capture" button, as that's the only thing that really needs "root". If we can't read the events or the tracers, then they will just stay empty or set to a single default value. No reason to disable export and import settings, the user can do that, but it wont help them much. -- Steve > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > kernel-shark/src/KsCaptureDialog.cpp | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp > index 2e6e8f9..2962917 100644 > --- a/kernel-shark/src/KsCaptureDialog.cpp > +++ b/kernel-shark/src/KsCaptureDialog.cpp > @@ -74,6 +74,19 @@ KsCaptureControl::KsCaptureControl(QWidget *parent) > _topLayout.addWidget(errorLabel); > > lamAddLine(); > + > + /* Disable all key buttons. */ > + QVector<QWidget *> widgets = > + {&_importSettingsButton, > + &_exportSettingsButton, > + &_outputBrowseButton, > + &_pluginsComboBox, > + &_commandCheckBox, > + &_applyButton, > + &_captureButton}; > + > + for (auto &b: widgets) > + b->setDisabled(true); > } > > pluginList.prepend("nop");