AU: Changes for deltas on traditional bios machines.
BUG=None
TEST=Attached unittests/tested on image
- Fix uninitialized variable err in action processor unittest
- Let Omaha dictate if an update is a delta or full update
- Bug fix in delta generator for differently-sized images
- More logging when applying delta updates
- Fix infinite loop in http fetcher unittest
- log each HTTP connection to know when a dropped connection is
reestablished.
- Detect when speed goes below a threshold and reestablish HTTP
connection (currently < 10bytes/sec for 90 contiguous seconds).
- Fix stack overflow in libcurl http fetcher.
- optimize out a lot of needless CPU usage in libcurl http fetcher
(turns out adding a glib main loop source uses a lot of CPU).
- subprocess: pass PATH, log stdout/stderr
- postinstall runner: support for ext3 and ext4 target filesystems.
Review URL: http://codereview.chromium.org/2805027
diff --git a/action_processor_unittest.cc b/action_processor_unittest.cc
index 0657207..40275a0 100644
--- a/action_processor_unittest.cc
+++ b/action_processor_unittest.cc
@@ -61,7 +61,11 @@
class MyActionProcessorDelegate : public ActionProcessorDelegate {
public:
explicit MyActionProcessorDelegate(const ActionProcessor* processor)
- : processor_(processor), processing_done_called_(false) {}
+ : processor_(processor),
+ processing_done_called_(false),
+ processing_stopped_called_(false),
+ action_completed_called_(false),
+ action_completed_success_(false) {}
virtual void ProcessingDone(const ActionProcessor* processor, bool success) {
EXPECT_EQ(processor_, processor);
diff --git a/delta_diff_generator.cc b/delta_diff_generator.cc
index 23c5fc8..ec07119 100644
--- a/delta_diff_generator.cc
+++ b/delta_diff_generator.cc
@@ -27,6 +27,7 @@
#include "update_engine/utils.h"
using std::make_pair;
+using std::max;
using std::min;
using std::set;
using std::string;
@@ -756,7 +757,7 @@
TEST_AND_RETURN_FALSE(utils::FileSize(old_kernel_part) >= 0);
TEST_AND_RETURN_FALSE(utils::FileSize(new_kernel_part) >= 0);
- vector<Block> blocks(min(old_image_stbuf.st_size / kBlockSize,
+ vector<Block> blocks(max(old_image_stbuf.st_size / kBlockSize,
new_image_stbuf.st_size / kBlockSize));
LOG(INFO) << "invalid: " << Vertex::kInvalidIndex;
LOG(INFO) << "len: " << blocks.size();
diff --git a/delta_performer.cc b/delta_performer.cc
index 1f8d8e4..8cf0226 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -193,6 +193,8 @@
next_operation_num_ - manifest_.install_operations_size());
if (!CanPerformInstallOperation(op))
break;
+ LOG(INFO) << "Performing operation " << next_operation_num_ << "/"
+ << total_operations;
bool is_kernel_partition =
(next_operation_num_ >= manifest_.install_operations_size());
if (op.type() == DeltaArchiveManifest_InstallOperation_Type_REPLACE ||
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index 89999eb..fe12b9b 100755
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -74,8 +74,8 @@
void CompareFilesByBlock(const string& a_file, const string& b_file) {
vector<char> a_data, b_data;
- EXPECT_TRUE(utils::ReadFile(a_file, &a_data));
- EXPECT_TRUE(utils::ReadFile(b_file, &b_data));
+ EXPECT_TRUE(utils::ReadFile(a_file, &a_data)) << "file failed: " << a_file;
+ EXPECT_TRUE(utils::ReadFile(b_file, &b_data)) << "file failed: " << b_file;
EXPECT_EQ(a_data.size(), b_data.size());
size_t kBlockSize = 4096;
diff --git a/http_fetcher_unittest.cc b/http_fetcher_unittest.cc
index a0e620b..4cabd1c 100644
--- a/http_fetcher_unittest.cc
+++ b/http_fetcher_unittest.cc
@@ -82,12 +82,21 @@
return;
}
int rc = 1;
+ int tries = 10;
+ started_ = true;
while (0 != rc) {
+ LOG(INFO) << "running wget to start";
rc = system((string("wget --output-document=/dev/null ") +
LocalServerUrlForPath("/test")).c_str());
+ LOG(INFO) << "done running wget to start";
usleep(10 * 1000); // 10 ms
+ tries--;
+ if (tries == 0) {
+ LOG(ERROR) << "Unable to start server.";
+ started_ = false;
+ break;
+ }
}
- started_ = true;
free(argv[0]);
return;
}
@@ -95,8 +104,10 @@
if (!started_)
return;
// request that the server exit itself
+ LOG(INFO) << "running wget to exit";
int rc = system((string("wget -t 1 --output-document=/dev/null ") +
LocalServerUrlForPath("/quitquitquit")).c_str());
+ LOG(INFO) << "done running wget to exit";
if (validate_quit_)
EXPECT_EQ(0, rc);
waitpid(pid_, NULL, 0);
@@ -111,7 +122,7 @@
public:
HttpFetcher* NewLargeFetcher() {
LibcurlHttpFetcher *ret = new LibcurlHttpFetcher;
- ret->set_idle_ms(1); // speeds up test execution
+ ret->set_idle_ms(1000); // speeds up test execution
return ret;
}
HttpFetcher* NewSmallFetcher() {
@@ -181,6 +192,25 @@
g_main_loop_unref(loop);
}
+TYPED_TEST(HttpFetcherTest, SimpleBigTest) {
+ GMainLoop *loop = g_main_loop_new(g_main_context_default(), FALSE);
+ {
+ HttpFetcherTestDelegate delegate;
+ delegate.loop_ = loop;
+ scoped_ptr<HttpFetcher> fetcher(this->NewLargeFetcher());
+ fetcher->set_delegate(&delegate);
+
+ typename TestFixture::HttpServer server;
+ ASSERT_TRUE(server.started_);
+
+ StartTransferArgs start_xfer_args = {fetcher.get(), this->BigUrl()};
+
+ g_timeout_add(0, StartTransfer, &start_xfer_args);
+ g_main_loop_run(loop);
+ }
+ g_main_loop_unref(loop);
+}
+
namespace {
class PausingHttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index 9ed0c64..7a032fe 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -19,6 +19,7 @@
}
void LibcurlHttpFetcher::ResumeTransfer(const std::string& url) {
+ LOG(INFO) << "Starting/Resuming transfer";
CHECK(!transfer_in_progress_);
url_ = url;
curl_multi_handle_ = curl_multi_init();
@@ -49,9 +50,15 @@
CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_WRITEFUNCTION,
StaticLibcurlWrite), CURLE_OK);
CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_URL, url_.c_str()), CURLE_OK);
+
+ // If the connection drops under 10 bytes/sec for 90 seconds, reconnect.
+ CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_LOW_SPEED_LIMIT, 10),
+ CURLE_OK);
+ CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_LOW_SPEED_TIME, 90),
+ CURLE_OK);
+
CHECK_EQ(curl_multi_add_handle(curl_multi_handle_, curl_handle_), CURLM_OK);
transfer_in_progress_ = true;
- CurlPerformOnce();
}
// Begins the transfer, which must not have already been started.
@@ -59,15 +66,16 @@
transfer_size_ = -1;
bytes_downloaded_ = 0;
resume_offset_ = 0;
- ResumeTransfer(url);
+ do {
+ ResumeTransfer(url);
+ } while (CurlPerformOnce());
}
void LibcurlHttpFetcher::TerminateTransfer() {
CleanUp();
}
-// TODO(adlr): detect network failures
-void LibcurlHttpFetcher::CurlPerformOnce() {
+bool LibcurlHttpFetcher::CurlPerformOnce() {
CHECK(transfer_in_progress_);
int running_handles = 0;
CURLMcode retcode = CURLM_CALL_MULTI_PERFORM;
@@ -82,7 +90,8 @@
CleanUp();
if ((transfer_size_ >= 0) && (bytes_downloaded_ < transfer_size_)) {
- ResumeTransfer(url_);
+ // Need to restart transfer
+ return true;
} else {
if (delegate_) {
delegate_->TransferComplete(this, true); // success
@@ -92,6 +101,7 @@
// set up callback
SetupMainloopSources();
}
+ return false;
}
size_t LibcurlHttpFetcher::LibcurlWrite(void *ptr, size_t size, size_t nmemb) {
@@ -163,7 +173,6 @@
// If we are already tracking this fd, continue.
if (io_channels_.find(i) != io_channels_.end())
continue;
-
// We must track a new fd
GIOChannel *io_channel = g_io_channel_unix_new(i);
guint tag = g_io_add_watch(
@@ -173,6 +182,11 @@
&StaticFDCallback,
this);
io_channels_[i] = make_pair(io_channel, tag);
+ static int io_counter = 0;
+ io_counter++;
+ if (io_counter % 50 == 0) {
+ LOG(INFO) << "io_counter = " << io_counter;
+ }
}
// Wet up a timeout callback for libcurl
@@ -186,51 +200,46 @@
// curl_multi_perform() again.
ms = idle_ms_;
}
- if (timeout_source_) {
- g_source_destroy(timeout_source_);
- timeout_source_ = NULL;
+ if (!timeout_source_) {
+ LOG(INFO) << "setting up timeout source:" << ms;
+ timeout_source_ = g_timeout_source_new(1000);
+ CHECK(timeout_source_);
+ g_source_set_callback(timeout_source_, StaticTimeoutCallback, this,
+ NULL);
+ g_source_attach(timeout_source_, NULL);
+ static int counter = 0;
+ counter++;
+ if (counter % 50 == 0) {
+ LOG(INFO) << "counter = " << counter;
+ }
}
- timeout_source_ = g_timeout_source_new(ms);
- CHECK(timeout_source_);
- g_source_set_callback(timeout_source_, StaticTimeoutCallback, this,
- NULL);
- g_source_attach(timeout_source_, NULL);
}
bool LibcurlHttpFetcher::FDCallback(GIOChannel *source,
GIOCondition condition) {
- // Figure out which source it was; hopefully there aren't too many b/c
- // this is a linear scan of our channels
- bool found_in_set = false;
- for (IOChannels::iterator it = io_channels_.begin();
- it != io_channels_.end(); ++it) {
- if (it->second.first == source) {
- // We will return false from this method, meaning that we shouldn't keep
- // this g_io_channel around. So we remove it now from our collection of
- // g_io_channels so that the other code in this class doens't mess with
- // this (doomed) GIOChannel.
- // TODO(adlr): optimize by seeing if we should reuse this GIOChannel
- g_source_remove(it->second.second);
- g_io_channel_unref(it->second.first);
- io_channels_.erase(it);
- found_in_set = true;
- break;
- }
+ while (CurlPerformOnce()) {
+ ResumeTransfer(url_);
}
- CHECK(found_in_set);
- CurlPerformOnce();
- return false;
+ // We handle removing of this source elsewhere, so we always return true.
+ // The docs say, "the function should return FALSE if the event source
+ // should be removed."
+ // http://www.gtk.org/api/2.6/glib/glib-IO-Channels.html#GIOFunc
+ return true;
}
-bool LibcurlHttpFetcher::TimeoutCallback() {
+gboolean LibcurlHttpFetcher::TimeoutCallback() {
+ if (!transfer_in_progress_)
+ return TRUE;
// Since we will return false from this function, which tells glib to
// destroy the timeout callback, we must NULL it out here. This way, when
// setting up callback sources again, we won't try to delete this (doomed)
// timeout callback then.
// TODO(adlr): optimize by checking if we can keep this timeout callback.
- timeout_source_ = NULL;
- CurlPerformOnce();
- return false;
+ //timeout_source_ = NULL;
+ while (CurlPerformOnce()) {
+ ResumeTransfer(url_);
+ }
+ return TRUE;
}
void LibcurlHttpFetcher::CleanUp() {
diff --git a/libcurl_http_fetcher.h b/libcurl_http_fetcher.h
index 6f32183..fd55d8e 100644
--- a/libcurl_http_fetcher.h
+++ b/libcurl_http_fetcher.h
@@ -69,7 +69,7 @@
return reinterpret_cast<LibcurlHttpFetcher*>(data)->FDCallback(source,
condition);
}
- bool TimeoutCallback();
+ gboolean TimeoutCallback();
static gboolean StaticTimeoutCallback(gpointer data) {
return reinterpret_cast<LibcurlHttpFetcher*>(data)->TimeoutCallback();
}
@@ -79,7 +79,8 @@
// one call to curl_multi_perform. This method will set up the glib run
// loop with sources for future work that libcurl will do.
// This method will not block.
- void CurlPerformOnce();
+ // Returns true if we should resume immediately after this call.
+ bool CurlPerformOnce();
// Sets up glib main loop sources as needed by libcurl. This is generally
// the file descriptor of the socket and a timer in case nothing happens
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 927c7d3..9d53a4e 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -289,6 +289,8 @@
output_object.needs_admin =
XmlGetProperty(updatecheck_node, "needsadmin") == "true";
output_object.prompt = XmlGetProperty(updatecheck_node, "Prompt") == "true";
+ output_object.is_delta =
+ XmlGetProperty(updatecheck_node, "IsDelta") == "true";
SetOutputObject(output_object);
return;
}
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 0f066e3..307798b 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -42,6 +42,7 @@
off_t size;
bool needs_admin;
bool prompt;
+ bool is_delta;
};
COMPILE_ASSERT(sizeof(off_t) == 8, off_t_not_64bit);
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
old mode 100644
new mode 100755
index cd91f68..cc37d7a
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -46,6 +46,7 @@
"appid=\"") + app_id + "\" status=\"ok\"><ping "
"status=\"ok\"/><updatecheck DisplayVersion=\"" + display_version + "\" "
"MoreInfo=\"" + more_info_url + "\" Prompt=\"" + prompt + "\" "
+ "IsDelta=\"true\" "
"codebase=\"" + codebase + "\" "
"hash=\"" + hash + "\" needsadmin=\"" + needsadmin + "\" "
"size=\"" + size + "\" status=\"ok\"/></app></gupdate>";
@@ -229,6 +230,7 @@
EXPECT_EQ("1.2.3.4", response.display_version);
EXPECT_EQ("http://code/base", response.codebase);
EXPECT_EQ("http://more/info", response.more_info_url);
+ EXPECT_TRUE(response.is_delta);
EXPECT_EQ("HASH1234=", response.hash);
EXPECT_EQ(123, response.size);
EXPECT_FALSE(response.needs_admin);
@@ -398,6 +400,7 @@
EXPECT_EQ("1.2.3.4", response.display_version);
EXPECT_EQ("http://code/base", response.codebase);
EXPECT_EQ("", response.more_info_url);
+ EXPECT_FALSE(response.is_delta);
EXPECT_EQ("HASH1234=", response.hash);
EXPECT_EQ(123, response.size);
EXPECT_TRUE(response.needs_admin);
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 319c825..d93919a 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -35,7 +35,7 @@
install_plan_.kernel_install_path =
utils::BootKernelDevice(install_plan_.install_path);
- install_plan_.is_full_update = true; // TODO(adlr): know if update is a delta
+ install_plan_.is_full_update = !response.is_delta;
TEST_AND_RETURN(HasOutputPipe());
if (HasOutputPipe())
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 489d5e7..f751bbe 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -88,6 +88,7 @@
in.size = 12;
in.needs_admin = true;
in.prompt = false;
+ in.is_delta = false;
InstallPlan install_plan;
EXPECT_TRUE(DoTest(in, "/dev/sda3", &install_plan));
EXPECT_TRUE(install_plan.is_full_update);
@@ -105,9 +106,10 @@
in.size = 12;
in.needs_admin = true;
in.prompt = true;
+ in.is_delta = true;
InstallPlan install_plan;
EXPECT_TRUE(DoTest(in, "/dev/sda5", &install_plan));
- EXPECT_TRUE(install_plan.is_full_update);
+ EXPECT_FALSE(install_plan.is_full_update);
EXPECT_EQ(in.codebase, install_plan.download_url);
EXPECT_EQ(in.hash, install_plan.download_hash);
EXPECT_EQ("/dev/sda3", install_plan.install_path);
@@ -122,6 +124,7 @@
in.size = 12;
in.needs_admin = true;
in.prompt = true;
+ in.is_delta = false;
InstallPlan install_plan;
EXPECT_TRUE(DoTest(in, "/dev/sda3", &install_plan));
EXPECT_TRUE(install_plan.is_full_update);
diff --git a/postinstall_runner_action.cc b/postinstall_runner_action.cc
index e8f5cc2..281582a 100644
--- a/postinstall_runner_action.cc
+++ b/postinstall_runner_action.cc
@@ -24,11 +24,6 @@
const string install_device = install_plan.install_path;
ScopedActionCompleter completer(processor_, this);
- utils::BootLoader boot_loader;
- TEST_AND_RETURN(utils::GetBootloader(&boot_loader));
-
- bool read_only = (boot_loader == utils::BootLoader_CHROME_FIRMWARE);
-
// Make mountpoint
string temp_dir;
TEST_AND_RETURN(utils::MakeTempDirectory("/tmp/au_postint_mount.XXXXXX",
@@ -37,14 +32,22 @@
{
// Scope for the mount
- unsigned long mountflags = read_only ? MS_RDONLY : 0;
+ unsigned long mountflags = MS_RDONLY;
int rc = mount(install_device.c_str(),
temp_dir.c_str(),
- "ext3",
+ "ext4",
mountflags,
NULL);
if (rc < 0) {
+ LOG(INFO) << "Failed to mount install part as ext4. Trying ext3.";
+ rc = mount(install_device.c_str(),
+ temp_dir.c_str(),
+ "ext3",
+ mountflags,
+ NULL);
+ }
+ if (rc < 0) {
LOG(ERROR) << "Unable to mount destination device " << install_device
<< " onto " << temp_dir;
return;
diff --git a/subprocess.cc b/subprocess.cc
old mode 100644
new mode 100755
index 87d8253..696779f
--- a/subprocess.cc
+++ b/subprocess.cc
@@ -35,11 +35,42 @@
argv[i] = NULL;
}
}
+
+// Note: Caller responsible for free()ing the returned value!
+char** ArgPointer() {
+ const char* keys[] = {"LD_LIBRARY_PATH", "PATH"};
+ char** ret = new char*[arraysize(keys) + 1];
+ int pointer = 0;
+ for (size_t i = 0; i < arraysize(keys); i++) {
+ ret[i] = NULL;
+ if (getenv(keys[i])) {
+ ret[pointer] = strdup(StringPrintf("%s=%s", keys[i],
+ getenv(keys[i])).c_str());
+ pointer++;
+ }
+ }
+ return ret;
+}
+
+class ScopedFreeArgPointer {
+ public:
+ ScopedFreeArgPointer(char** arr) : arr_(arr) {}
+ ~ScopedFreeArgPointer() {
+ if (!arr_)
+ return;
+ for (int i = 0; arr_[i]; i++)
+ free(arr_[i]);
+ delete[] arr_;
+ }
+ private:
+ char** arr_;
+ DISALLOW_COPY_AND_ASSIGN(ScopedFreeArgPointer);
+};
} // namespace {}
uint32_t Subprocess::Exec(const std::vector<std::string>& cmd,
- ExecCallback callback,
- void* p) {
+ ExecCallback callback,
+ void* p) {
GPid child_pid;
GError* err;
scoped_array<char*> argv(new char*[cmd.size() + 1]);
@@ -48,13 +79,8 @@
}
argv[cmd.size()] = NULL;
- scoped_array<char*> argp(new char*[2]);
- argp[0] = argp[1] = NULL;
- const char* kLdLibraryPathKey = "LD_LIBRARY_PATH";
- if (getenv(kLdLibraryPathKey)) {
- argp[0] = strdup(StringPrintf("%s=%s", kLdLibraryPathKey,
- getenv(kLdLibraryPathKey)).c_str());
- }
+ char** argp = ArgPointer();
+ ScopedFreeArgPointer argp_free(argp);
SubprocessCallbackRecord callback_record;
callback_record.callback = callback;
@@ -62,7 +88,7 @@
bool success = g_spawn_async(NULL, // working directory
argv.get(),
- argp.get(),
+ argp,
G_SPAWN_DO_NOT_REAP_CHILD, // flags
NULL, // child setup function
NULL, // child setup data pointer
@@ -93,8 +119,12 @@
argv[i] = strdup(cmd[i].c_str());
}
argv[cmd.size()] = NULL;
- char* argp[1];
- argp[0] = NULL;
+
+ char** argp = ArgPointer();
+ ScopedFreeArgPointer argp_free(argp);
+
+ char* child_stdout;
+ char* child_stderr;
bool success = g_spawn_sync(NULL, // working directory
argv.get(),
@@ -102,13 +132,17 @@
static_cast<GSpawnFlags>(NULL), // flags
NULL, // child setup function
NULL, // data for child setup function
- NULL, // return location for stdout
- NULL, // return location for stderr
+ &child_stdout,
+ &child_stderr,
return_code,
&err);
FreeArgv(argv.get());
if (err)
LOG(INFO) << "err is: " << err->code << ", " << err->message;
+ if (child_stdout)
+ LOG(INFO) << "Postinst stdout:" << child_stdout;
+ if (child_stderr)
+ LOG(INFO) << "Postinst stderr:" << child_stderr;
return success;
}