----------------------------------------------------------- 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 > >
