Integrate new file ioslave in KIO job
ClosedPublic

Authored by chinmoyr on Jul 22 2017, 12:22 PM.

Details

Summary
  1. Adds two new job flags, PrivilegeExecution and NoPrivilegeExecution.
  2. Adds code for confirmation dialog to be shown when the slave asks for it.
  3. Provides connection for privilegeOperationRequested emmited by the slave.

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Jul 22 2017, 12:22 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 22 2017, 12:22 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
chinmoyr edited the summary of this revision. (Show Details)Jul 22 2017, 2:46 PM
chinmoyr added reviewers: dfaure, Frameworks.
dfaure requested changes to this revision.Jul 24 2017, 6:01 PM
dfaure added inline comments.
src/core/job.cpp
345

This hardcoded 5 is awful, please use SlaveBase::Continue instead.

src/core/job_base.h
222

This doesn't need to be public, does it? Could be in the private class.

229

This doesn't need to be public, does it? Could be in the private class.

src/core/job_p.h
94

Please keep all the member variables together. In the orig code it's already ugly that there are methods after the variables, but this makes it slightly worse with the enum in the middle and no empty line between vars and methods.

Ideally this should be
[...] private enum, private methods, all private member vars, end of class

195

typo: emitted

src/core/simplejob.cpp
151

This is not a very good signal name.
Signals notify a state changed and usually end with "ed".
Would it make sense to call this privilegeOperationRequested() or something?
I need to find more about when this is emitted and what the data is ;)

346

use QLatin1String rather than QStringLiteral for comparisons

354

OK, so the data is two possible strings, and the reply is two possible strings, why not use enums instead? You can define them in SlaveBase for instance.

You might have to use lambdas to avoid the enum appearing in Q_PRIVATE_SLOT in public headers (and requiring slavebase.h)

This revision now requires changes to proceed.Jul 24 2017, 6:01 PM
chinmoyr updated this revision to Diff 17222.Jul 26 2017, 10:16 AM
chinmoyr edited edge metadata.
  1. Separated enums, vars and functions
  2. Removed hardcoding
  3. Use the new signal privilegeOperationRequested
  4. Removed public methods added in previous revision. Instead of them d_func is used (which should have been the case right from the start but I overlooked it somehow)
dfaure requested changes to this revision.Jul 27 2017, 7:52 AM

Looks good, apart from a few minor things.

src/core/job.cpp
275

its -> it's

src/core/job_p.h
198

Private header -> no need for @since doc, it's not part of the API.

src/core/simplejob.cpp
346

I'm not sure if the bool->int conversion is safe and guaranteed (AFAIK, in theory a bool can convert to 2 or anything else that is not 0, depending on how it was set in the first place)

This would be safer if it said QByteArray::number(confirmed ? 1 : 0), but then that's even better written as confirmed ? "1" : "0", no need to call int->string conversion code;

This revision now requires changes to proceed.Jul 27 2017, 7:52 AM
chinmoyr updated this revision to Diff 18018.Aug 11 2017, 7:00 PM
chinmoyr edited edge metadata.
  • Fixed the issues pointed out.
  • Check for UnitTesting key in metadata while showing the confirmation dialog.
dfaure added inline comments.Aug 11 2017, 9:19 PM
src/core/simplejob.cpp
346

Check that this is consistent with SlaveBase...

chinmoyr updated this revision to Diff 18702.Aug 24 2017, 5:40 PM
  • Return an enum (PrivilegeOperationStatus) instead of bool for confirmation status.
dfaure accepted this revision.Aug 24 2017, 7:28 PM
This revision is now accepted and ready to land.Aug 24 2017, 7:28 PM
chinmoyr updated this revision to Diff 20454.Oct 7 2017, 4:53 PM

Added NoPrivilegeExecution flag to opt-out privilege execution.

dfaure requested changes to this revision.Oct 7 2017, 5:19 PM
dfaure added inline comments.
src/core/job_base.h
311

You can use @since 5.40 everywhere...

317

typo: insufficient

I would add "without attempting to run the operation as root first".

320

It's usually just */ on this line (single star)

This revision now requires changes to proceed.Oct 7 2017, 5:19 PM
chinmoyr updated this revision to Diff 20486.Oct 8 2017, 4:53 PM

minox changes

chinmoyr updated this revision to Diff 24597.Jan 2 2018, 3:35 PM
This comment was removed by chinmoyr.
chinmoyr updated this revision to Diff 24598.Jan 2 2018, 3:36 PM

1.Reassigned flag values for NoPrivilegeExecution and PrivilegeExecution. I strongly feel that the latter won't be needed anymore.
2.Setting meta-data for unit test in the job itself (in tryAskPrivilegeOpConfirmation).
3.Changed some comments due to changing flags .

dfaure accepted this revision.Jan 10 2018, 11:30 PM

Please keep in mind that we should remove one of the two flags before the end of the month, after people tested this as much as possible ;)

I admit that it sucks a bit that I don't know what the default should be...
Conservatively, I think opt-in is better, since the worst case is an app that regularly does a failing copy in the background, and it would now prompt the user every time... well, maybe this doesn't happen anywhere though ;)

Anyhow, let's first get this in. Thanks for your perseverance!

This revision is now accepted and ready to land.Jan 10 2018, 11:30 PM
chinmoyr updated this revision to Diff 25222.Jan 12 2018, 12:00 PM
chinmoyr edited the summary of this revision. (Show Details)

Summary update
Fixed windows build

This revision was automatically updated to reflect the committed changes.