----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125613/#review87242 -----------------------------------------------------------
Ship it! The fix looks fine to me now, thanks. The unittest can be improved a bit, feel free to commit after making the changes suggested below (or tell me if you have no account) kio/tests/jobtest.cpp (line 374) <https://git.reviewboard.kde.org/r/125613/#comment59923> missing spaces around '=' sign kio/tests/jobtest.cpp (line 395) <https://git.reviewboard.kde.org/r/125613/#comment59924> bitfield operations on booleans are not a good idea. I wish there was a &&=, like you but (at least in C++98) there isn't, so the only solution is ok_copyjob = ok_copyjob && ... But anyway the whole idea of using "and" means that on failure we don't know which condition failed. Why not QVERIFY() both statements separately? kio/tests/jobtest.cpp (line 413) <https://git.reviewboard.kde.org/r/125613/#comment59925> alternatively, you could use QSignalSpy, then you don't need a slot :-) - David Faure On Oct. 19, 2015, 9:43 a.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. 19, 2015, 9:43 a.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/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 > >
