DevHeads.net

Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet

Review request for kdelibs and David Faure.

Summary (updated)
Do not call ftpSendMimeType for empty files in Ftp::ftpGet

Bugs: 323491
<a href="http://bugs.kde.org/show_bug.cgi?id=323491" title="http://bugs.kde.org/show_bug.cgi?id=323491">http://bugs.kde.org/show_bug.cgi?id=323491</a>

Repository: kdelibs

Description (updated)
The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened.

Diffs (updated)
kioslave/ftp/ftp.cpp 5bb2e8d

Diff: <a href="https://git.reviewboard.kde.org/r/116523/diff/" title="https://git.reviewboard.kde.org/r/116523/diff/">https://git.reviewboard.kde.org/r/116523/diff/</a>

Testing (updated)
Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server.

Thanks,

Dawit Alemayehu

Comments

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/12/2014 - 06:35

(Updated March 12, 2014, 11:35 a.m.)

Review request for kdelibs and David Faure.

Changes
Updated patch based on feedback.

Bugs: 323491
<a href="http://bugs.kde.org/show_bug.cgi?id=323491" title="http://bugs.kde.org/show_bug.cgi?id=323491">http://bugs.kde.org/show_bug.cgi?id=323491</a>

Repository: kdelibs

Description
The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened.

Diffs (updated)
kioslave/ftp/ftp.cpp ddc6eaf

Diff: <a href="https://git.reviewboard.kde.org/r/116523/diff/" title="https://git.reviewboard.kde.org/r/116523/diff/">https://git.reviewboard.kde.org/r/116523/diff/</a>

Testing
Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server.

Thanks,

Dawit Alemayehu

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/12/2014 - 06:39

(Updated March 12, 2014, 11:39 a.m.)

Review request for kdelibs and David Faure.

Changes
Reverted incorrect patch update.

Bugs: 323491
<a href="http://bugs.kde.org/show_bug.cgi?id=323491" title="http://bugs.kde.org/show_bug.cgi?id=323491">http://bugs.kde.org/show_bug.cgi?id=323491</a>

Repository: kdelibs

Description
The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened.

Diffs (updated)
kioslave/ftp/ftp.cpp ddc6eaf

Diff: <a href="https://git.reviewboard.kde.org/r/116523/diff/" title="https://git.reviewboard.kde.org/r/116523/diff/">https://git.reviewboard.kde.org/r/116523/diff/</a>

Testing
Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server.

Thanks,

Dawit Alemayehu

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/12/2014 - 07:47

(Updated March 12, 2014, 12:47 p.m.)

Review request for kdelibs and David Faure.

Changes
Updated patch to send the proper mimetype for completely empty files (0 bytes).

Bugs: 323491
<a href="http://bugs.kde.org/show_bug.cgi?id=323491" title="http://bugs.kde.org/show_bug.cgi?id=323491">http://bugs.kde.org/show_bug.cgi?id=323491</a>

Repository: kdelibs

Description
The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened.

Diffs (updated)
kioslave/ftp/ftp.cpp ddc6eaf

Diff: <a href="https://git.reviewboard.kde.org/r/116523/diff/" title="https://git.reviewboard.kde.org/r/116523/diff/">https://git.reviewboard.kde.org/r/116523/diff/</a>

Testing
Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server.

Thanks,

Dawit Alemayehu

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/12/2014 - 08:26

(Updated March 12, 2014, 1:26 p.m.)

Review request for kdelibs and David Faure.

Changes
updated patch.

Bugs: 323491
<a href="http://bugs.kde.org/show_bug.cgi?id=323491" title="http://bugs.kde.org/show_bug.cgi?id=323491">http://bugs.kde.org/show_bug.cgi?id=323491</a>

Repository: kdelibs

Description
The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened.

Diffs (updated)
kioslave/ftp/ftp.cpp ddc6eaf

Diff: <a href="https://git.reviewboard.kde.org/r/116523/diff/" title="https://git.reviewboard.kde.org/r/116523/diff/">https://git.reviewboard.kde.org/r/116523/diff/</a>

Testing
Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server.

Thanks,

Dawit Alemayehu

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/13/2014 - 00:05

(Updated March 13, 2014, 5:05 a.m.)

Status
This change has been marked as submitted.

Review request for kdelibs and David Faure.

Bugs: 323491
<a href="http://bugs.kde.org/show_bug.cgi?id=323491" title="http://bugs.kde.org/show_bug.cgi?id=323491">http://bugs.kde.org/show_bug.cgi?id=323491</a>

Repository: kdelibs

Description
The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened.

Diffs
kioslave/ftp/ftp.cpp ddc6eaf

Diff: <a href="https://git.reviewboard.kde.org/r/116523/diff/" title="https://git.reviewboard.kde.org/r/116523/diff/">https://git.reviewboard.kde.org/r/116523/diff/</a>

Testing
Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server.

Thanks,

Dawit Alemayehu

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Commit Hook at 03/13/2014 - 00:05

This review has been submitted with commit 2dbcc83c25d9b2a8a20b517973f332cf67f57518 by Dawit Alemayehu to branch KDE/4.12.

- Commit Hook

On March 12, 2014, 1:26 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By David Faure at 03/12/2014 - 09:39

Ship it!

Thanks.

I hope we don't get into the case of "Unknown Size" when the size is actually 0, though. That case would still be buggy, and I'm not sure how we could fix it.

- David Faure

On March 12, 2014, 1:26 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/13/2014 - 00:01

Well I could think of one scenario where that could potentially happen. The FTP server does not respond correctly to the SIZE command and the size of the file we are trying to retrieve is actually 0, but I am sure that is a rare server like the one reported in the very old bug report #168011.

- Dawit

On March 12, 2014, 1:26 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By David Faure at 03/12/2014 - 07:54

kioslave/ftp/ftp.cpp
<https://git.reviewboard.kde.org/r/116523/#comment37197>

A bit hard to read compared to
if (m_size != 0) { while(true) { ... } }
because m_size doesn't change, so it's weird to read "while the size is not 0" - it either was, or it will never be.

kioslave/ftp/ftp.cpp
<https://git.reviewboard.kde.org/r/116523/#comment37198>

the alternative to the if+while is to just if(m_size==0) { mimeType(zerosize); return; } before the while.

- David Faure

On March 12, 2014, 12:47 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By David Faure at 03/04/2014 - 14:42

I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug?

- David Faure

On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By David Faure at 03/12/2014 - 06:30

No, application/x-zerosize.

(defaultMimeType means unknown)

- David

On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/12/2014 - 06:29

Ahh... I forgot that the mimeType signal MUST be emitted. But even if we avoid the while loop the if statement also prevents the emission of that signal. What mime-type should be emitted in case of zero sized files would the better question I guess. KMimeType::defaultMimeType()?

- Dawit

On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By David Faure at 03/12/2014 - 02:15

I still think this is wrong. By contract the kioslave must emit a mimetype.
Instead, ftpSendMimeType should be fixed, to detect the case where totalSize is 0, and skip the loop in that case since we have nothing to read.

- David

On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/11/2014 - 23:15

dfaure: any further questions on this?

- Dawit

On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote:

Re: Review Request 116523: Do not call ftpSendMimeType for empty

By Dawit A at 03/05/2014 - 07:43

It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement "if (!buffer.isEmpty())" will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files.

- Dawit

On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: