-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125613/#review87048
-----------------------------------------------------------
Thanks for the investigation and patch.
kio/kio/copyjob.cpp (line 579)
<https://git.reviewboard.kde.org/r/125613/#comment59824>
We don't want to abort the copy just because one unreadable folder was found. We want to tell the user (hence the warning line below) and keep going.
This emulates what `cp -a ~/src ~/dst` does.
OK, you're not exactly aborting, but you're still setting an error - even though everything else got copied. I don't see why this is necessary. Is it just to get an error box, or does it fix behaviour?
kio/kio/copyjob.cpp (line 582)
<https://git.reviewboard.kde.org/r/125613/#comment59830>
This is what is supposed to show you a message box about the fact that a subdirectory was skipped.
If it doesn't work, I would suggest debugging that (maybe a problem with the default ui delegate).
kio/kio/job.cpp (line 2659)
<https://git.reviewboard.kde.org/r/125613/#comment59829>
this makes sense indeed.
kio/kio/job.cpp (line 2669)
<https://git.reviewboard.kde.org/r/125613/#comment59828>
isn't this the error message you're getting?
I'm confused then (you're adding code that isn't getting called?)
kio/kio/job.cpp (line 2671)
<https://git.reviewboard.kde.org/r/125613/#comment59833>
Same question here. As the comment says, "the result is still OK", which is why I didn't set an error code here. Please explain why this is needed.
kio/kio/job.cpp (line 2683)
<https://git.reviewboard.kde.org/r/125613/#comment59832>
I see. SimpleJob::slotFinished() will emit result again, if it happens after this.
This check is unusual, but then again, most other composite jobs do not also run a slave themselves.
Can you add a comment? Like
// if the main directory listing is still running, it will emit result in SimpleJob::slotFinished()
kio/tests/jobtest.cpp (line 377)
<https://git.reviewboard.kde.org/r/125613/#comment59825>
Just call QDir().remove always, you don't check its return value anyway ;)
kio/tests/jobtest.cpp (line 399)
<https://git.reviewboard.kde.org/r/125613/#comment59826>
Typo: premissions -> permissions
kio/tests/jobtest.cpp (line 411)
<https://git.reviewboard.kde.org/r/125613/#comment59827>
please remove trailing space everywhere
- David Faure
On Oct. 17, 2015, 2:02 p.m., Alberto Jiménez Ruiz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125613/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2015, 2:02 p.m.)
>
>
> Review request for kdelibs, Albert Astals Cid and David Faure.
>
>
> Bugs: 333436
> http://bugs.kde.org/show_bug.cgi?id=333436
>
>
> Repository: kdelibs
>
>
> Description
> -------
>
> Race condition and error notification loss in ListJob
>
>
> Diffs
> -----
>
> kio/kio/copyjob.cpp e6c3817
> kio/kio/job.cpp 91712e3
> kio/kio/jobclasses.h d771cfe
> kio/tests/jobtest.h d3c552e
> kio/tests/jobtest.cpp ee2677a
>
> Diff: https://git.reviewboard.kde.org/r/125613/diff/
>
>
> Testing
> -------
>
> Update 1, added unittest
> Changed condition of listjob slotresult finishing (When executed as a synchronous job, gets stuck in a eventloop)
> Added setError to copyjob.
>
>
> Tests done on Kubuntu 15.10:
>
> With this folder structure in ~:
> src
> ######c (Folder)
> ##############b (Folder chmod'ed to 700 and owned by root)
> #######################d (10MB file made of /dev/urandom data)
> ##############a (10MB file made of /dev/urandom data)
>
> Then, (as a normal user execute: "kioclient ~/src ~/dst")
>
> Expected result: Notification that some files were not able to be copied (Cannot access "b" folder)
> Result with latest kdelibs: Silent copying where b folder is owned by user but nothing inside.
> ---When all kdebugdialog flags are set, a backtrace is also seen.
>
> subError notifications for listjob subjobs are lost, Copyjob uses this signal to skip data.
>
> Also, this fix prevents a race condition.
>
> ListJob has another ListJob as a subjob.
>
> Chaild fails for whatever reason and calls error, which calls slotError, which calls slotsFinished and emitResult.
> The result of the child gets collected by parent. slotResult of parent gets called, removes subjob and emitsresult because there are no subjobs left.
> ---That's fine as long as the slave associated with the parent emits finished before the child fails. If it doesn't, parent calls emitsResult twice.
>
>
> Result after patch: kioclient shows a MessageBox telling the copy operation could not be completed.
>
>
> Thanks,
>
> Alberto Jiménez Ruiz
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20151019/cf27a6fe/attachment.htm>