D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 25222. chinmoyr edited the summary of this revision. chinmoyr added a comment. Summary update Fixed windows build REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=24598=25222 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h src/ioslaves/file/file.cpp To: chinmoyr, dfaure, #frameworks Cc: mreeves, #frameworks
D6832: Integrate new file ioslave in KIO job
This revision was automatically updated to reflect the committed changes. Closed by commit R241:596ef4ccf1d7: Integrate new file ioslave in KIO job (authored by chinmoyr). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D6832?vs=25222=25223#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=25222=25223 REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h src/ioslaves/file/file.cpp To: chinmoyr, dfaure, #frameworks Cc: mreeves, #frameworks
D6832: Integrate new file ioslave in KIO job
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. 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! REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: mreeves, #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr added a comment. ping REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: mreeves, #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 24598. chinmoyr added a comment. 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 . REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=24597=24598 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h To: chinmoyr, dfaure, #frameworks Cc: mreeves, #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 24597. chinmoyr added a comment. 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 . REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=20486=24597 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/global.h src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h src/core/slavebase.cpp src/core/slavebase.h src/core/slaveinterface.cpp src/core/slaveinterface.h src/ioslaves/file/CMakeLists.txt src/ioslaves/file/fdreceiver.cpp src/ioslaves/file/fdreceiver.h src/ioslaves/file/file.cpp src/ioslaves/file/file.h src/ioslaves/file/file_p.h src/ioslaves/file/file_unix.cpp src/ioslaves/file/file_win.cpp src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/fdsender.cpp src/ioslaves/file/kauth/fdsender.h src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h src/ioslaves/file/sharefd_p.h To: chinmoyr, dfaure, #frameworks Cc: mreeves, #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 20486. chinmoyr added a comment. minox changes CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=20454=20486 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > job_base.h:311 > + * > + * @since 5.39 > + **/ You can use @since 5.40 everywhere... > job_base.h:317 > + * When set, notifies the slave that application/job does not want > privilege execution. > + * So in case of failure due to insufficint privileges show an error. > + * typo: insufficient I would add "without attempting to run the operation as root first". > job_base.h:320 > + * @since 5.39 > + **/ > +NoPrivilegeExecution = 16 It's usually just */ on this line (single star) REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 20454. chinmoyr added a comment. Added `NoPrivilegeExecution` flag to opt-out privilege execution. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=18702=20454 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
dfaure accepted this revision. This revision is now accepted and ready to land. BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 18702. chinmoyr added a comment. - Return an enum (PrivilegeOperationStatus) instead of bool for confirmation status. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=18018=18702 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
dfaure added inline comments. INLINE COMMENTS > simplejob.cpp:346 > +bool confirmed = tryAskPrivilegeOpConfirmation(); > +m_slave->send(MSG_PRIVILEGE_EXEC, QByteArray(confirmed ? "1" : "0")); > +} Check that this is consistent with SlaveBase... REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 18018. chinmoyr added a comment. - Fixed the issues pointed out. - Check for UnitTesting key in metadata while showing the confirmation dialog. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=17222=18018 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Looks good, apart from a few minor things. INLINE COMMENTS > job.cpp:276 > +// Whether or not sub-jobs have PrivilegeExecution flag set > doesn't matter. > +// If its not set in parent job then don't continue. > +return false; its -> it's > job_p.h:197 > + * > + * @since 5.37 > + */ Private header -> no need for @since doc, it's not part of the API. > simplejob.cpp:346 > +bool confirmed = tryAskPrivilegeOpConfirmation(); > +m_slave->send(MSG_PRIVILEGE_EXEC, QByteArray::number(confirmed)); > +} 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; REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr updated this revision to Diff 17222. chinmoyr added a comment. 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) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6832?vs=17021=17222 BRANCH temp REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> job.cpp:345
> +m_confirmationAsked = true;
> +return status == 5;
> +}
This hardcoded 5 is awful, please use SlaveBase::Continue instead.
> job_base.h:222
> + */
> +bool isPrivilegeExecutionEnabled() const;
> +
This doesn't need to be public, does it? Could be in the private class.
> job_base.h:229
> + */
> +bool wasConfirmationAsked() const;
> +
This doesn't need to be public, does it? Could be in the private class.
> job_p.h:94
> +FileOperationType m_operationType;
> +bool tryAskPrivilegeOpConfirmation();
> +
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
> job_p.h:195
> /**
> + * Called when privilegeExecData() is emmited by the slave.
> + *
typo: emitted
> simplejob.cpp:151
>
> +q->connect(slave, SIGNAL(privilegeExecData(QByteArray)),
> + SLOT(slotPrivilegeExecData(QByteArray)));
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 ;)
> simplejob.cpp:346
> +QByteArray reply;
> +if (data == QStringLiteral("CanElevatePrivilege")) {
> +if (m_privilegeExecutionEnabled
use QLatin1String rather than QStringLiteral for comparisons
> simplejob.cpp:354
> +if (confirmed) {
> +reply = QByteArray("ActionConfirmed");
> +}
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)
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D6832
To: chinmoyr, dfaure, #frameworks
Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr added a dependent revision: D6833: Add support for PrivilegeExecution in KIO jobs. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr added reviewers: dfaure, Frameworks. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr added a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr removed a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr added a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr Cc: #frameworks
D6832: Integrate new file ioslave in KIO job
chinmoyr created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY 1. Adds a new job flag, PrivilegeExecution, to be used from applications side. 2. Adds code for confirmation dialog to be shown when the slave asks for it. 3. Provides connection for privilegeExecData emmited by the slave. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6832 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h To: chinmoyr Cc: #frameworks
