Authorize using Polkit backend, compile Helper as standalone application and send progress via QDBus from helper towards application
Needs ReviewPublic

Authored by shubham on Fri, Aug 2, 8:51 AM.

Diff Detail

Repository
R16 KPMCore
Branch
auth-polkit-backend
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15191
Build 15209: arc lint + arc unit
shubham requested review of this revision.Fri, Aug 2, 8:51 AM
shubham created this revision.
shubham edited the summary of this revision. (Show Details)Fri, Aug 2, 8:52 AM
shubham added reviewers: stikonas, cjlcarvalho.
shubham added a project: KDE Partition Manager.
shubham added a subscriber: KDE Partition Manager.

Before it's pushed (once it compiles) also refactor code a bit so that
there isn't that much code duplication.

shubham updated this revision to Diff 62977.Fri, Aug 2, 2:46 PM

Refactor

@stikonas It says error: ‘org’ does not name a type

auto interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"),

I think I need to add ExternalCommand as an interface.

Yes, and you need to add it to cmake. That part of code is autogenerated.

shubham updated this revision to Diff 63036.EditedSun, Aug 4, 6:30 AM

Fix errors, make it compile
@stikonas can you have a look please?

shubham updated this revision to Diff 63037.Sun, Aug 4, 6:42 AM

connect to newData signal in the application

shubham retitled this revision from [WIP]: Send progress through QDBus from helper towards application to [WIP]: Send progress via QDBus from helper towards application.Sun, Aug 4, 6:44 AM
stikonas added inline comments.Sun, Aug 4, 10:14 AM
src/util/externalcommandhelper.cpp
261–285

Most of this code (except for asyncCall is the same in all cases).

It would make sense to factor it out into a separate function which might e.g. take an argument that is passed to emitNewData.

If you do just this you might still need two function (one for int overload that passed progress percents, one for the other call with QStrings).

It might be possible to unify it more but at leat do that...

@stikonas Yes, I am planning to re utilize the code, my first intention was just to make it compile.

shubham updated this revision to Diff 63135.Mon, Aug 5, 4:55 PM

Refactor

shubham updated this revision to Diff 63137.Mon, Aug 5, 5:17 PM

Rename function

shubham marked an inline comment as done.Mon, Aug 5, 5:17 PM
cjlcarvalho added inline comments.Mon, Aug 5, 5:23 PM
src/util/externalcommandhelper.cpp
60

What happens when a service/object is registered and the other one is not?

broulik added a subscriber: broulik.Mon, Aug 5, 5:28 PM
broulik added inline comments.
src/util/externalcommandhelper.cpp
59

One should register objects before registering the service as the service might become visible to the bus in an inconsistent state (i.e. show up before the objects are registered)

155

This is a lie.

160

QDBusInterface constructor blocks introspecting DBus. You probably want to use an interface generated from an XML file to avoid this.

171

What's the point of the event loop?

199

You're leaking the watcher, call watcher->deleteLater() in the finished handler

234–236

This probably wants to be a QElapsedTimer

shubham updated this revision to Diff 63139.Mon, Aug 5, 5:50 PM
shubham marked 4 inline comments as done.
This revision was not accepted when it landed; it landed in state Needs Review.Mon, Aug 5, 5:56 PM
This revision was automatically updated to reflect the committed changes.
shubham reopened this revision.Mon, Aug 5, 6:04 PM
shubham updated this revision to Diff 63160.Tue, Aug 6, 8:24 AM

Remove unwanted signals

shubham retitled this revision from [WIP]: Send progress via QDBus from helper towards application to Authorize using Polkit backend, compile Helper as standalone application and send progress via QDBus from helper towards application.Wed, Aug 7, 12:16 PM
shubham edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Wed, Aug 7, 12:31 PM
This revision was automatically updated to reflect the committed changes.
shubham reopened this revision.Sat, Aug 10, 7:27 PM
shubham updated this revision to Diff 63501.Sat, Aug 10, 7:28 PM

QDBus communication

stikonas requested changes to this revision.Tue, Aug 13, 11:13 PM
stikonas added inline comments.
src/util/externalcommand_polkitbackend.cpp
157

What sets m_result? in the connect call above you simply pass result to QEventLoop's function quit without processing it.

This revision now requires changes to proceed.Tue, Aug 13, 11:13 PM
stikonas added inline comments.Tue, Aug 13, 11:15 PM
src/util/externalcommand_polkitbackend.cpp
97–108

Why do we need connect to finished signal if in the line above you wait for call to finish with call.waitForFinished() that function blocks until call is done.

shubham updated this revision to Diff 63739.Wed, Aug 14, 4:07 PM
shubham marked 2 inline comments as done.

Try to fix the broken things

shubham marked an inline comment as not done.Wed, Aug 14, 5:00 PM
shubham added inline comments.
src/util/externalcommand_polkitbackend.cpp
157

Actually yes, it is currently broke, however it does not matter, because I am just using this function to show the auth agent and if you notice line 383 externalcommand.cpp, to display some log message. Oh, now I get why it was reporting ""Unable to obtain Administrative privileges, the action can not be executed!!";"

shubham updated this revision to Diff 63746.Wed, Aug 14, 5:30 PM
shubham marked an inline comment as not done.

Try fixing the inline comments

shubham updated this revision to Diff 63747.Wed, Aug 14, 5:32 PM

some more changes

shubham marked an inline comment as done.Wed, Aug 14, 5:33 PM
shubham updated this revision to Diff 63754.Wed, Aug 14, 6:19 PM

Move helperMain() outside Q_SLOTS

shubham updated this revision to Diff 63879.Fri, Aug 16, 5:59 PM
  1. Fix runtime warnings inside initPolkitAgent()
  2. Use "new" inside ctor to instantiate object