D10635: Bug 383764: remove last file being copied/moved when the action was canceled or when the disk was full

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  The idea kind of makes sense, but as is, it's very dangerous, what if the destination already existed?
  
  Case 1: copy file:///dir1 to ftp://host/dir2 which already exists. During the 'stat src' or 'stat dest' phase, cancel the job -> it will now delete dir2 !
  (see copyjob.cpp:879 which sets m_currentDestURL).
  
  Case 2: *move* dir1 to dir2 which already exists, cancel before the move happens, boom.
  
  I don't have time to think about more cases, but I suspect there are some more.
  
  Unittesting "cancelling at this exact step of the copy" sounds very tricky, unfortunately... but maybe we can add unittest-specific env vars to abort with a disk full error at some points in the code.
  
  Also, why is this done at the PasteJob level? there are many other ways to trigger a copy, including drag-n-drop. It sounds like this should rather be done inside CopyJob, which would also make it possible to have more information about whether it's safe to clean up or not.

INLINE COMMENTS

> copyjob.h:99
> +     * Returns the current file destination URL.
> +     * @return the current file destination URL
> +     */

@since 5.44 if it has to be public, but please also document what this really is, for people who haven't read the copyjob.cpp source code.

> dropjob.cpp:559
>  {
> -    if (job->error()) {
> -        KIO::Job::slotResult(job); // will set the error and emit result(this)
> -        return;
> +    int jobError = job->error();
> +    if (jobError) {

const int...

> dropjob.cpp:563
> +            // Must remove last file that was not finished moving/copying
> +            KIO::CopyJob *copyJob = qobject_cast<KIO::CopyJob *>(job);
> +            this->addSubjob(KIO::del(copyJob->currentDestUrl(), JobFlag::HideProgressInfo));

Since you don't test the result of the cast (I assume because by construction only a CopyJob is connected to this slot?), just use static_cast.

> dropjob.cpp:564
> +            KIO::CopyJob *copyJob = qobject_cast<KIO::CopyJob *>(job);
> +            this->addSubjob(KIO::del(copyJob->currentDestUrl(), JobFlag::HideProgressInfo));
> +            return;

this-> isn't really useful, I'd remove it

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10635

To: meven, dfaure
Cc: ltoscano, ngraham, #frameworks, michaelh
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180219/8f2c2c0e/attachment.html>