On Fri, 9 Aug 2019 11:06:22 +0300
"Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:
> If KernelShark GUI has been started as Root we do not need to use
> "pkexec" when starting the Record dialog. Note that the actual place
> where "pkexec" gets used is in the script "kshark-su-record".
>
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@xxxxxxxxx>
> ---
> kernel-shark/src/KsMainWindow.cpp | 47 +++++++++++++++++++++----------
> 1 file changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 2560bf8..e9c6d54 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -883,23 +883,26 @@ void KsMainWindow::_pluginAdd()
>
> void KsMainWindow::_record()
> {
> -#ifndef DO_AS_ROOT
> + bool canDoAsRoot(false);
Is this the C++ way of doing:
bool canDoAsRoot = false; ?
>
> - QErrorMessage *em = new QErrorMessage(this);
> - QString message;
> -
> - message = "Record is currently not supported.";
> - message += " Install \"pkexec\" and then do:<br>";
> - message += " cd build <br> sudo ./cmake_uninstall.sh <br>";
> - message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
> - message += " sudo make install";
> +#ifdef DO_AS_ROOT
BTW, I think we should rename DO_AS_ROOT to "HAS_PKEXEC" as that is
much more descriptive of what that macro means.
> + canDoAsRoot = true;
> +#endif
>
> - em->showMessage(message);
> - qCritical() << "ERROR: " << message;
> + if (geteuid() && !canDoAsRoot) {
> + QErrorMessage *em = new QErrorMessage(this);
> + QString message;
>
> - return;
> + message = "Record is currently not supported.";
> + message += " Install \"pkexec\" and then do:<br>";
> + message += " cd build <br> sudo ./cmake_uninstall.sh <br>";
> + message += " ./cmake_clean.sh <br> cmake .. <br> make <br>";
> + message += " sudo make install";
>
> -#endif
> + em->showMessage(message);
> + qCritical() << "ERROR: " << message;
> + return;
> + }
>
> _capture.start();
> }
> @@ -1134,9 +1137,24 @@ void KsMainWindow::loadSession(const QString &fileName)
>
> void KsMainWindow::_initCapture()
> {
> + bool canDoAsRoot(false);
> +
> #ifdef DO_AS_ROOT
> + canDoAsRoot = true;
> +#endif
As we are repeating this, why not just do at the top of the file:
#ifdef HAS_PKEXEC
# define has_pkexec 1
#else
# define has_pkexec 0
#endif
And remove the deplicate logic.
> +
> + if (geteuid() && !canDoAsRoot)
> + return;
>
> - _capture.setProgram("kshark-su-record");
> + if (geteuid()) {
Also, "geteuid()" is a system call. It causes a call to the kernel each
time. We should cache that in a local variable instead, and use that.
uid_t euid = geteuid();
Then use "euid" for other locations. But its OK to calculated it in the
function itself. That is, don't use the value from a different
function, as we don't want to worry about adding commands that change
the euid later.
-- Steve
> + _capture.setProgram("kshark-su-record");
> + } else {
> + QStringList argv;
> +
> + _capture.setProgram("kshark-record");
> + argv << QString("-o ") + QDir::homePath();
> + _capture.setArguments(argv);
> + }
>
> connect(&_capture, &QProcess::started,
> this, &KsMainWindow::_captureStarted);
> @@ -1155,7 +1173,6 @@ void KsMainWindow::_initCapture()
> connect(&_captureLocalServer, &QLocalServer::newConnection,
> this, &KsMainWindow::_readSocket);
>
> -#endif
> }
>
> void KsMainWindow::_captureStarted()
|