Storage Framework

Merge lp:~michihenning/storage-framework/fix-linkat into lp:storage-framework/devel

  1. fix-linkat
  2. Merge into devel
Proposed by Michi Henning
Status: Merged
Approved by: James Henstridge
Approved revision: 37
Merged at revision: 38
Proposed branch: lp:~michihenning/storage-framework/fix-linkat
Merge into: lp:storage-framework/devel
Diff against target: 188 lines (+67/-19)
2 files modified
include/unity/storage/qt/client/internal/local_client/UploaderImpl.h (+3/-0)
src/qt/client/internal/local_client/UploaderImpl.cpp (+64/-19)
To merge this branch: bzr merge lp:~michihenning/storage-framework/fix-linkat
Reviewer Review Type Date Requested Status
James Henstridge Approve
unity-api-1-bot continuous-integration Needs Fixing
Review via email: mp+300958@code.launchpad.net

Commit message

Work-around for failing linkat() on Vivid/Arm.

Description of the change

Work-around for failing linkat() on Vivid/Arm.

To post a comment you must log in.
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

FAILED: Continuous integration, rev:36
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/35/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/244/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/252
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=vivid+overlay/187
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=xenial+overlay/187
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=yakkety/187
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/116/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/116/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

Mostly looks good, but it looks like the temp file will be left behind in case of an error.

review: Needs Fixing
lp:~michihenning/storage-framework/fix-linkat updated
37. By Michi Henning

Changed logic for handling temp file linking/renaming. We use O_TMPFILE and linkat()
if that works, and mkstemp() and rename(), otherwise.

Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

FAILED: Continuous integration, rev:37
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/38/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/252/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/260
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=vivid+overlay/195
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=xenial+overlay/195
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=yakkety/195
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/124/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/124/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/unity/storage/qt/client/internal/local_client/UploaderImpl.h'
2--- include/unity/storage/qt/client/internal/local_client/UploaderImpl.h 2016-07-22 00:17:24 +0000
3+++ include/unity/storage/qt/client/internal/local_client/UploaderImpl.h 2016-07-26 01:53:29 +0000
4@@ -57,6 +57,8 @@
5 std::weak_ptr<Root> root,
6 QFutureInterface<std::shared_ptr<File>>& qf,
7 QFutureInterface<void>& worker_initialized);
8+ virtual ~UploadWorker();
9+
10 void start_uploading() noexcept;
11
12 public Q_SLOTS:
13@@ -89,6 +91,7 @@
14 QFutureInterface<std::shared_ptr<File>>& qf_;
15 QFutureInterface<void>& worker_initialized_;
16 QString error_msg_;
17+ bool use_linkat_ = true;
18 };
19
20 class UploadThread : public QThread
21
22=== modified file 'src/qt/client/internal/local_client/UploaderImpl.cpp'
23--- src/qt/client/internal/local_client/UploaderImpl.cpp 2016-07-22 01:45:39 +0000
24+++ src/qt/client/internal/local_client/UploaderImpl.cpp 2016-07-26 01:53:29 +0000
25@@ -72,6 +72,14 @@
26 worker_initialized_.reportStarted();
27 }
28
29+UploadWorker::~UploadWorker()
30+{
31+ if (state_ == error && !use_linkat_ && output_file_)
32+ {
33+ output_file_->remove(); // LCOV_EXCL_LINE
34+ }
35+}
36+
37 void UploadWorker::start_uploading() noexcept
38 {
39 read_socket_.reset(new QLocalSocket);
40@@ -99,22 +107,28 @@
41 // Some kernels on the phones don't support O_TMPFILE and return various errno values when this fails.
42 // So, if anything at all goes wrong, we fall back on conventional temp file creation and
43 // produce a hard error if that doesn't work either.
44+ // Note that, in this case, the temp file retains its name in the file system. Not nice because,
45+ // if the client dies at the wrong moment, we leave the temp file behind.
46+ use_linkat_ = false;
47 string tmpfile = parent_path.native() + "/" + TMPFILE_PREFIX + "XXXXXX";
48 tmp_fd_.reset(mkstemp(const_cast<char*>(tmpfile.data())));
49 if (tmp_fd_.get() == -1)
50 {
51- error_msg_ = "Uploader: cannot create temp file \"" + QString::fromStdString(tmpfile)
52- + "\": " + QString::fromStdString(storage::internal::safe_strerror(errno));
53 worker_initialized_.reportFinished();
54- state_ = error;
55- do_finish();
56+ QString msg = "cannot create temp file \"" + QString::fromStdString(tmpfile)
57+ + "\": " + QString::fromStdString(storage::internal::safe_strerror(errno));
58+ handle_error(msg);
59 return;
60 }
61- unlink(tmpfile.data());
62+ output_file_.reset(new QFile(QString::fromStdString(tmpfile)));
63+ output_file_->open(QIODevice::WriteOnly);
64 // LCOV_EXCL_STOP
65 }
66- output_file_.reset(new QFile);
67- output_file_->open(tmp_fd_.get(), QIODevice::WriteOnly, QFileDevice::DontCloseHandle);
68+ else
69+ {
70+ output_file_.reset(new QFile);
71+ output_file_->open(tmp_fd_.get(), QIODevice::WriteOnly, QFileDevice::DontCloseHandle);
72+ }
73
74 worker_initialized_.reportFinished();
75 }
76@@ -128,7 +142,7 @@
77 {
78 if (qf_.future().isFinished())
79 {
80- return; // Future was set previously. no point in continuing.
81+ return; // Future was set previously, no point in continuing.
82 }
83
84 switch (state_)
85@@ -187,13 +201,15 @@
86 auto bytes_written = output_file_->write(buf);
87 if (bytes_written == -1)
88 {
89- handle_error("socket error: " + output_file_->errorString());
90+ handle_error("socket error: " + output_file_->errorString()); // LCOV_EXCL_LINE
91 }
92 else if (bytes_written != buf.size())
93 {
94+ // LCOV_EXCL_START
95 QString msg = "write error, requested " + QString::number(buf.size()) + " B, but wrote only "
96 + bytes_written + " B.";
97 handle_error(msg);
98+ // LCOV_EXCL_STOP
99 }
100 }
101 }
102@@ -220,6 +236,7 @@
103
104 if (impl->has_conflict())
105 {
106+ state_ = error;
107 make_exceptional_future(qf_, ConflictException("Uploader::finish_upload(): ETag mismatch"));
108 return;
109 }
110@@ -230,6 +247,7 @@
111 int fd = open(path_.toStdString().c_str(), O_WRONLY | O_CREAT | O_EXCL, 0600); // Fails if path already exists.
112 if (fd == -1)
113 {
114+ state_ = error;
115 QString msg = "Uploader::finish_upload(): item with name \"" + path_ + "\" exists already";
116 QString name = QString::fromStdString(boost::filesystem::path(path_.toStdString()).filename().native());
117 make_exceptional_future(qf_, ExistsException(msg, path_, name));
118@@ -237,10 +255,13 @@
119 }
120 if (close(fd) == -1)
121 {
122+ // LCOV_EXCL_START
123+ state_ = error;
124 QString msg = "Uploader::finish_upload(): cannot close tmp file: "
125 + QString::fromStdString(storage::internal::safe_strerror(errno));
126 make_exceptional_future(qf_, ResourceException(msg));
127 return;
128+ // LCOV_EXCL_STOP
129 }
130
131 file = FileImpl::make_file(path_, root_);
132@@ -249,22 +270,46 @@
133
134 if (!output_file_->flush())
135 {
136+ // LCOV_EXCL_START
137+ state_ = error;
138 QString msg = "Uploader::finish_upload(): cannot flush output file: " + output_file_->errorString();
139 make_exceptional_future(qf_, ResourceException(msg));
140 return;
141+ // LCOV_EXCL_STOP
142 }
143
144 // Link the anonymous tmp file into the file system.
145- string oldpath = string("/proc/self/fd/") + std::to_string(tmp_fd_.get());
146- string newpath = file->native_identity().toStdString();
147- ::unlink(newpath.c_str()); // linkat() will not remove existing file: http://lwn.net/Articles/559969/
148- if (linkat(-1, oldpath.c_str(), tmp_fd_.get(), newpath.c_str(), AT_SYMLINK_FOLLOW) == -1)
149- {
150- state_ = error;
151- QString msg = "Uploader::finish_upload(): linkat \"" + file->native_identity() + "\" failed: "
152- + QString::fromStdString(storage::internal::safe_strerror(errno));
153- make_exceptional_future(qf_, ResourceException(msg));
154- return;
155+ auto new_path = file->native_identity().toStdString();
156+ if (use_linkat_)
157+ {
158+ auto old_path = string("/proc/self/fd/") + std::to_string(tmp_fd_.get());
159+ ::unlink(new_path.c_str()); // linkat() will not remove existing file: http://lwn.net/Articles/559969/
160+ if (linkat(-1, old_path.c_str(), tmp_fd_.get(), new_path.c_str(), AT_SYMLINK_FOLLOW) == -1)
161+ {
162+ // LCOV_EXCL_START
163+ state_ = error;
164+ QString msg = "Uploader::finish_upload(): linkat \"" + QString::fromStdString(old_path)
165+ + "\" to \"" + file->native_identity() + "\" failed: "
166+ + QString::fromStdString(storage::internal::safe_strerror(errno));
167+ make_exceptional_future(qf_, ResourceException(msg));
168+ return;
169+ // LCOV_EXCL_STOP
170+ }
171+ }
172+ else
173+ {
174+ // LCOV_EXCL_START
175+ auto old_path = output_file_->fileName().toStdString();
176+ if (rename(old_path.c_str(), new_path.c_str()) == -1)
177+ {
178+ state_ = error;
179+ QString msg = "Uploader::finish_upload(): rename \"" + QString::fromStdString(old_path)
180+ + "\" to \"" + file->native_identity() + "\" failed: "
181+ + QString::fromStdString(storage::internal::safe_strerror(errno));
182+ make_exceptional_future(qf_, ResourceException(msg));
183+ return;
184+ }
185+ // LCOV_EXCL_STOP
186 }
187
188 state_ = finalized;

Subscribers

People subscribed via source and target branches

to all changes: