Merge lp:~charlesk/keeper/fix-slow-tar-creator-test into lp:keeper/devel

[Charles Kerr]

Fix TarCreator::step() to block less

Have TarCreator::step() take smaller steps so that in practice the function won't block.

Also, fix side bug: step() wasn't handling for the case of archive_write_data() returning a non-error status of having written less than the full buffer size.

[unity-api-1-bot (unity-api-1-bot)]

FAILED: Continuous integration, rev:143
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~charlesk/keeper/fix-slow-tar-creator-test/+merge/311984/+edit-commit-message

https://jenkins.canonical.com/unity-api-1/job/lp-keeper-ci/143/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1199
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1206
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/992
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/992/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/992
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/992/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/992
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/992/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/992
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/992/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/992
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/992/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/992
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/992/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-keeper-ci/143/rebuild

review: Needs Fixing (continuous-integration)

[unity-api-1-bot (unity-api-1-bot)]

PASSED: Continuous integration, rev:
https://jenkins.canonical.com/unity-api-1/job/lp-keeper-ci/144/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1206
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1213
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/999
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/999/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/999
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/999/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/999
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/999/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/999
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/999/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/999
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/999/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/999
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/999/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-keeper-ci/144/rebuild

review: Approve (continuous-integration)

[Xavi Garcia (xavi-garcia-mena)]

Thanks for this, Charles...
It works much better that it did.

review: Approve

=== modified file 'src/tar/tar-creator.cpp'
--- src/tar/tar-creator.cpp	2016-09-05 18:38:36 +0000
+++ src/tar/tar-creator.cpp	2016-11-28 21:38:06 +0000
@@ -78,24 +78,20 @@
             step_filenum_ = -1;
         }
 
-        for(;;)
+        // if we don't have a file we're working on, then get one
+        if (!step_file_)
         {
-            // if we don't have a file we're working on, then get one
-            if (!step_file_)
-            {
-                if (step_filenum_ >= filenames_.size()) // tried to read past the end
-                {
-                    success = false;
-                    break;
-                }
-
-                // step to next file
-                if (++step_filenum_ == filenames_.size()) // we made it to the end!
-                {
-                    archive_write_close(step_archive_.get());
-                    break;
-                }
-
+            if (step_filenum_ >= filenames_.size()) // tried to read past the end
+            {
+                success = false;
+            }
+            // step to next file
+            else if (++step_filenum_ == filenames_.size()) // we made it to the end!
+            {
+                archive_write_close(step_archive_.get());
+            }
+            else
+            {
                 // write the file's header
                 const auto& filename = filenames_[step_filenum_];
                 add_file_header_to_archive(step_archive_.get(), filename);
@@ -104,15 +100,24 @@
                 step_file_.reset(new QFile(filename));
                 step_file_->open(QIODevice::ReadOnly);
             }
+        }
 
+        if (step_file_)
+        {
             static constexpr int BUFSIZE {1024*10};
             char inbuf[BUFSIZE];
-            const auto n = step_file_->read(inbuf, sizeof(inbuf));
-            if (n > 0) // got data
+            auto inbuf_len = step_file_->read(inbuf, sizeof(inbuf));
+            if (inbuf_len > 0) // got data
             {
-                for(;;) {
-                    if (archive_write_data(step_archive_.get(), inbuf, size_t(n)) != -1)
-                        break;
+                decltype(inbuf_len) offset = 0;
+                while(offset < inbuf_len) {
+                    auto const n_written = archive_write_data(step_archive_.get(), inbuf+offset, inbuf_len-offset);
+                    if (n_written > 0) {
+                        offset += n_written;
+                        if (offset == inbuf_len)
+                            break;
+                        continue;
+                    }
                     const auto err = archive_errno(step_archive_.get());
                     if (err == ARCHIVE_RETRY)
                         continue;
@@ -125,22 +130,19 @@
                         throw std::runtime_error(errstr.toStdString());
                 }
             }
-            else if (n < 0) // read error
+            else if (inbuf_len < 0) // read error
             {
                 success = false;
                 auto errstr = QStringLiteral("read()ing %1 returned %2 (%3)")
                                   .arg(step_file_->fileName())
-                                  .arg(n)
+                                  .arg(inbuf_len)
                                   .arg(step_file_->errorString());
                 qWarning() << errstr;
                 throw std::runtime_error(errstr.toStdString());
             }
-            else if (step_file_->atEnd()) // eof
-            {
-                // loop to next file
+
+            if (step_file_->atEnd()) // if we're done with the file, close it
                 step_file_.reset();
-                continue;
-            }
         }
 
         std::swap(fillme,step_buf_);