| Message ID | 20190809080623.7548-3-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Handle the case when KernelShark is started as Root | expand |
On Fri, 9 Aug 2019 11:06:22 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> 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@gmail.com> > --- > 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()
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); - 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 + 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 + + if (geteuid() && !canDoAsRoot) + return; - _capture.setProgram("kshark-su-record"); + if (geteuid()) { + _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()
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@gmail.com> --- kernel-shark/src/KsMainWindow.cpp | 47 +++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 15 deletions(-)