This revision was automatically updated to reflect the committed changes.
Closed by commit R241:17bf6df0d8ba: Add kauth helper to file ioslave (authored
by chinmoyr).
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=20608=25217
REVISION DETAIL
chinmoyr reopened this revision.
This revision is now accepted and ready to land.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:29a741982fe5: Add kauth helper to file ioslave (authored
by chinmoyr).
CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D6197?vs=20310=20608#toc
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
dfaure accepted this revision.
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr updated this revision to Diff 20310.
chinmoyr added a comment.
replaced occurence of sharefd.{h, cpp} with fdsender.{h, cpp}
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=18504=20310
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6197
AFFECTED
dfaure added a comment.
(strange that Phabricator still says "Needs Review" when this has two
approvals)
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
elvisangelaccio accepted this revision as: elvisangelaccio.
elvisangelaccio added inline comments.
INLINE COMMENTS
> file.actions:3
> +Name=Execute action as root.
> +Description=Root privileges are required to complete the action.
> +Policy=auth_admin
We could use more descriptive sentences
dfaure accepted this revision.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr updated this revision to Diff 18504.
chinmoyr added a comment.
- Minor changes in sendFileDescriptor function because constructor of
FdSender changed in https://phabricator.kde.org/D6709.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=18301=18504
BRANCH
master
chinmoyr updated this revision to Diff 18301.
chinmoyr added a comment.
-Removed the hard-coded socket path. Now it will take socket path from
ioslave.
-Removed sendFileDescriptor(-1) calls.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=18300=18301
BRANCH
master
chinmoyr updated this revision to Diff 18300.
chinmoyr added a comment.
- Removed the hard-coded socket path. Now it will take socket path from
ioslave.
- Removed sendFileDescriptor(-1) calls.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=18235=18300
BRANCH
master
chinmoyr added a comment.
> Is it even needed to send -1, if we're going to return an error reply
anyway?
The current logic is "after a successful sendFileDescriptor close the
socket". So sending -1 was to ensure socket is closed.
REVISION DETAIL
https://phabricator.kde.org/D6197
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filehelper.cpp:34
> +FdSender fdSender;
> +fdSender.connectToPath("org_kde_kio_file_helper_socket");
> +if (fdSender.isConnected()) {
BTW what
chinmoyr added inline comments.
INLINE COMMENTS
> dfaure wrote in filehelper.cpp:46
> Well, this proves exactly my earlier point: we should only test errno *when*
> a libc function fails.
>
> If this code is still checking errno after success somewhere, then *that* it
> what should be fixed.
chinmoyr updated this revision to Diff 18235.
chinmoyr added a comment.
- Remove errno assignment
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=18201=18235
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6197
AFFECTED FILES
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filehelper.cpp:46
> +// Set it to 0.
> +errno = 0;
> +
Well, this proves exactly my earlier point: we should only test errno *when* a
libc function
chinmoyr added inline comments.
INLINE COMMENTS
> dfaure wrote in filehelper.cpp:44
> should be unnecessary now, with the above suggested change
In my system errno is set to 11. It seems like the case where function call
succeeds but errno is set. But then it happens right after control
chinmoyr updated this revision to Diff 18201.
chinmoyr added a comment.
- fixed minor issues
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=18012=18201
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6197
AFFECTED FILES
src/ioslaves/file/CMakeLists.txt
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filehelper.cpp:39
> +}
> +errno = -1;
> +}
this basically abuses a global variable for something that could just be a
return value from this
chinmoyr updated this revision to Diff 18012.
chinmoyr added a comment.
Improved error checking in helper.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=17780=18012
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6197
AFFECTED FILES
dfaure added a comment.
Not sure I understand your question. If we agree on using return values then
there is only one way to do that. E.g. chmod() returns 0 on success, so the
code could be like
if (chmod(...) == 0)
return reply;
break;
which jumps to the if
chinmoyr added inline comments.
INLINE COMMENTS
> dfaure wrote in filehelper.cpp:86
> The documentation for chown and others says:
>
> Upon successful completion, these functions shall return 0. Otherwise, these
> functions shall return −1 and set errno to indicate the error.
>
> It does NOT
chinmoyr updated this revision to Diff 17780.
chinmoyr added a comment.
- Add private header
- minor fixes
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=17507=17780
BRANCH
first
REVISION DETAIL
https://phabricator.kde.org/D6197
AFFECTED FILES
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> chinmoyr wrote in filehelper.cpp:86
> In case of OPEN and OPENDIR, I can see why we need error checking, opendir()
> can return null and there are some
chinmoyr added inline comments.
INLINE COMMENTS
> dfaure wrote in filehelper.cpp:86
> opendir can return 0, in which case the next line will probably crash.
>
> Overall, this whole method seriously lacks error handling in my opinion. Just
> checking for errno at the end isn't enough, e.g. in
chinmoyr updated this revision to Diff 17507.
chinmoyr added a comment.
- used switch..case
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=17013=17507
REVISION DETAIL
https://phabricator.kde.org/D6197
AFFECTED FILES
src/ioslaves/file/CMakeLists.txt
chinmoyr added a dependent revision: D6829: Add ability to use the new kauth
helper in file ioslave.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr edited the summary of this revision.
chinmoyr added a dependency: D6709: [RFC] Add support for sharing file
descriptor between KIO slave and KAuth helper.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr added a dependent revision: D6831: Make use of kauth helper in methods
of file ioslave.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr added a dependent revision: D6830: Make use of kauth helper in copy
method of file ioslave.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr added a task: T6561: Polkit support in KIO.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr removed a task: T6561: Polkit support in KIO.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr added a task: T6561: Polkit support in KIO.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
chinmoyr updated this revision to Diff 17013.
chinmoyr retitled this revision from "Add basic KAuth support to file ioslave"
to "Add kauth helper to file ioslave".
chinmoyr edited the summary of this revision.
chinmoyr removed subscribers: davidedmundson, dfaure, eliasp, aacid.
chinmoyr added a
34 matches
Mail list logo
